-
Notifications
You must be signed in to change notification settings - Fork 259
feat: add sorting controls to count-plot chart #109
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
Conversation
Add ability to control sorting of count-plot bars from the UI:
- sortBy: sort by total count ("all") or selected/filtered count ("sel")
- sortOrder: ascending or descending order
Special values (null, other) remain at the bottom regardless of sort settings.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
|
Hi! We have a use case where we want to sort entries in count-plot chart by the amount of entries matching current selection (in our case it's coming from applying a filter on chart1 and ordering the other charts accordingly) Does it make sense? |
|
Makes sense. Can you execute the test plan and carefully review the code since you used ai to generate it? |
| sortBy: "total" | "selected" | undefined, | ||
| sortOrder: "asc" | "desc" | undefined, | ||
| ) { | ||
| // Split into regular items and special items (null, other) |
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.
This code iterates over the items two times. Would it be faster and simpler to use the special item index like before and then to use slice to split the array?
A unit test could also be good 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.
added a test
|
We group items with smaller total count into the "(other)" category when there are too many distinct values. If we sort by count in selection, it will be possible that the top selected count is an item in the "(other)" category. The current code seems to sort after the SQL query, which means the "(other)" category is considered as a unit to sort. Is this what you intend, or do you intend to see the top item under "(other)"? |
|
lemme know if smth else needs to be fixed |
|
Thanks for the contribution! We are incorporating this as part of #136. |
Summary
sortByoption: sort by total count ("all") or selected/filtered count ("sel")sortOrderoption: ascending (↑) or descending (↓) order