-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes table display bug when collection has one attribute and it is hidden #1836
Conversation
…idden. Fix is by showing the hidden attribute.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1836 +/- ##
==========================================
+ Coverage 85.51% 86.13% +0.62%
==========================================
Files 649 649
Lines 33276 33287 +11
Branches 9293 8770 -523
==========================================
+ Hits 28455 28671 +216
+ Misses 4463 4458 -5
+ Partials 358 158 -200
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
codap-v3
|
Project |
codap-v3
|
Branch Review |
main
|
Run status |
|
Run duration | 06m 43s |
Commit |
|
Committer | Evangeline Ireland |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
1
|
|
51
|
|
0
|
|
245
|
View all changes introduced in this branch ↗︎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments below for details/suggestions.
// if attribute is the only attribute in the collection and it is hidden, make the attribute visible | ||
if (attrs.length === 1 && caseMetadata?.isHidden(attrs[0].id)) { | ||
caseMetadata?.setIsHidden(attrs[0].id, false) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of issues:
- The case table is not the right place to address an issue that doesn't require the case table. (Attributes can be hidden from other components, e.g. the case card.)
- The accessor function of a reaction is just for returning values to the action function, not for actually changing models.
- The number of attributes isn't important. The relevant test is whether all remaining attributes in a collection are hidden, in which case they should all be shown.
I suspect that a reaction that is installed in the afterCreate()
of the SharedCaseMetadata
would make the most sense.
…ttributes as a reaction in the SharedCaseMetadata model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM -- just a couple of final suggested tweaks.
Fix is by showing the hidden attribute, which automatically makes copllection visible.