-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
chore: Improves the native filters UI/UX - iteration 5 #14882
chore: Improves the native filters UI/UX - iteration 5 #14882
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14882 +/- ##
==========================================
- Coverage 77.52% 77.51% -0.01%
==========================================
Files 963 963
Lines 49284 49297 +13
Branches 6203 6207 +4
==========================================
+ Hits 38206 38214 +8
- Misses 10877 10882 +5
Partials 201 201
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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. Some (mostly unrelated) random observations:
- Numeric range offers the option to sort values, despite them not really being relevant (the query always only returns one single row):
- The "Add filter" button triggers an action that seems to be pretty expensive and freezes the UI for a while. I believe it freezes while the filter config is populated. This could potentially be done async (didn't look at the specifics of what goes on)
- when the filter list is full, adding a new filter that falls outside the current view gets selected but isn't shown (we should probably scroll to the bottom when adding a new filter completes):
const FILTER_TYPE_NAME_MAPPING = { | ||
[t('Select filter')]: t('Value'), | ||
[t('Range filter')]: t('Numerical range'), | ||
[t('Time filter')]: t('Time range'), | ||
[t('Time column')]: t('Time column'), | ||
[t('Time grain')]: t('Time grain'), | ||
[t('Group By')]: t('Group by'), | ||
}; | ||
|
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.
In a subsequent pass we should probably add a TODO:
here for moving these names to the plugins.
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.
@villebro Thanks for the review. Let's tackle each item!
SUMMARY
Improves the native filters UI/UX - iteration 5.
This work is part of the Native dashboard filter project
The items below will be handled in the next iterations:
The iterations below are optional but recommended:
@villebro @rusackas @junlincc
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
20210527160830684.mp4
20210527155818663.mp4
TESTING INSTRUCTIONS
1 - Enable native filters feature flag
2 - Enter in a dashboard
3 - Add a native filter
4 - Check the screen
ADDITIONAL INFORMATION