Improvements to the Search Filters #1143
Conversation
Add accordion functionality to filter groups, changed how the system prioritizes which filters are open by default, added new sort logic for prioritizing certain filters. Fixes: HDX-2299, HDX-2302
🦋 Changeset detectedLatest commit: 6202ce9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| connectionId: chartConfig.connection, | ||
| }); | ||
|
|
||
| const { data: source } = useSource({ id: sourceId }); |
There was a problem hiding this comment.
Let me know your thoughts on this.. the parent does the same query (DBSearchPage) so we benefit from react query caching instead of passing props/storing in context
| tableMetadata: TableMetadata | undefined, | ||
| key: string, | ||
| ) { | ||
| return tableMetadata?.primary_key?.includes(key); |
There was a problem hiding this comment.
Note that tableMetadata?.primary_key? is a string (like ID, Column2, Key3).. so this could actually have false positives. We do this elsewhere in the app, so I kept the convention. However, I could add a utility to attempt to clean up this object (ex .split(', ')) and update all references if we want.
Stably Runner - Test Suite - 'Smoke Test'Test Suite Run Result: 🟢 Success (4/4 tests passed) [dashboard] This comment was generated from stably-runner-action |
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
Looks super useful! There are two things I noticed:
- Previously, we did not show "Load more" until "Show more" is clicked. Now we show both. Is that intentional?
- If it is intentional and we keep it that way, would it make sense to automatically "Show more" if the "Load more" is clicked? It seems odd to have to click "Load more" and then "Show more" to see what was just loaded.
Good idea, I made it so clicking "Show More" will automatically load more as well! |
@SpencerTorres @teeohhem I took a stab at the feedback from last week. Showing the count seemed too tight (not much room) so instead it will be slightly grayed out if no results exist. Also moved the search icon to the right and tightened up the spacing. |
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
LGTM, looks like a great improvement
teeohhem
left a comment
teeohhem
left a comment
There was a problem hiding this comment.
One final note: The chevron arrows aren't consistent with what we have on the search page.
">" is the default if there is content to expand
and
"V" is if content is expanded.
I realize this is inconsistent with "Load More" as well, bit it is confusing with the addition of the accordion event expansion with search tables now.
elizabetdev
left a comment
elizabetdev
left a comment
There was a problem hiding this comment.
Hi @brandon-pereira,
I noticed a couple of things that could be improved:
- If there’s only one item (or just a few), the load more button shouldn’t be displayed since all items are already visible.
- The search feels a bit confusing. My instinct was to click the search icon, but that closed the accordion. Having the search input inside the accordion title also felt odd, and sometimes the search icon overlaps with the title.
Suggestion
To address these issues, here’s what I’d suggest. Totally fine if you prefer to handle this in a separate PR or if you’d like me to take it on.
@elizabetdev Agreed! These all seem like good suggestions, thanks for taking the time to review this! Given timing, I have created another ticket (HDX-2366) to implement this feedback! Thanks again for reviewing 🙏 |
Add accordion functionality to filter groups, changed how the system prioritizes which filters are open by default, added new sort logic for prioritizing certain filters.
Logic as follows;
Fixes: HDX-2299, HDX-2302