-
Notifications
You must be signed in to change notification settings - Fork 4
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
[DT-791] Add count to top of data library #2758
Conversation
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.
Looks good, thank you, see optional comment inline 👍🏽
summary='faceted dataset search table' | ||
/> | ||
<> | ||
<div style={{ fontWeight: 600, borderBottom: '1px solid black' }}>{rowData.length} {_.capitalize(rowData.length > 1 ? tab.plural : tab.singular)}</div> |
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.
An interesting interaction is that if you filter to 0
records, the isEmpty(filteredData)
condition kicks in and this block, along with the table, disappears. I think it would be cleaner to reflect that condition on this line, but it certainly isn't required. In that case, if we did show 0
records, the plural would be the correct language to use here.
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.
I agree; I was expecting that the total would be outside the table, so it would be displayed in all cases. I don't see a specific UX proposal on the ticket so I may have missed a discussion about the UX in refinement.
Instead of showing a total inside the table, I think it would be better to show the total at the top, maybe on the same lines as the search (filter) text entry box. Also I would include both the number shown and the total number, so a user can tell how much of the total they're omitting. Text like "Showing X out of Y"/ "Showing all of Y"/"Showing none of Y" is one way to format this information.
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.
I think for now my inclination is to leave the present isEmpty
behavior, because it reduces redundant info. If necessary we can update it later.
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.
Overall looks good, are there mocks for this? Does this reflect the filtered datasets/studies, or all? Does this work for the specific branded libraries too?
@rjohanek I can't think of why branding would impact it - they should be using the same component? It does update when I filter. |
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.
Thanks for the updates 👍🏽
Addresses
Summary
Have you read Terra's Contributing Guide lately? If not, do that first.