-
Notifications
You must be signed in to change notification settings - Fork 16.6k
feat(show-badges): Dashboard level config for showing/hiding filter badge and cross filter badge #14249
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
Codecov Report
@@ Coverage Diff @@
## master #14249 +/- ##
==========================================
+ Coverage 76.11% 76.90% +0.78%
==========================================
Files 945 946 +1
Lines 47946 47987 +41
Branches 5950 5962 +12
==========================================
+ Hits 36494 36903 +409
+ Misses 11246 10883 -363
+ Partials 206 201 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| editMode: dashboardState.editMode, | ||
| isExpanded: !!dashboardState.expandedSlices[id], | ||
| showFilterBadge: dashboardInfo.metadata?.showFilterBadge ?? true, | ||
| showCrossFilterBadge: dashboardInfo.metadata?.showCrossFilterBadge ?? true, |
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 it's better to do hydrate file default value
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.
Yes, let's pick this up in the hydration phase.
| formData, | ||
| editMode: dashboardState.editMode, | ||
| isExpanded: !!dashboardState.expandedSlices[id], | ||
| showFilterBadge: dashboardInfo.metadata?.showFilterBadge ?? true, |
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.
may be it can good approach to switch it to hideFilterBadge instead and it will be used as undefined as false @villebro thoughts
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 it's usually better to avoid having negatives in option names. Let's just make sure we initialize with defaults and then make sure we do strict equality to avoid problems from undefined values.
| import_time = fields.Integer() | ||
| remote_id = fields.Integer() | ||
| showFilterBadge = fields.Boolean() | ||
| showCrossFilterBadge = fields.Boolean() |
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.
should it be snake case?
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.
Agreed, let's keep this consistent.
SUMMARY
There is a requirement to show/hide the existing badges in the slice/chart header
This feature is focused on optionally hiding the filter badge indicator and the cross filter indicator by configuring them from dashboard metadata
a follow-up PR might add UI checkboxes to configure them
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
show_badge.mov
TEST PLAN
backward compatibility:
0. DASHBOARD_CROSS_FILTERS
"showFilterBadge"
"showCrossFilterBadge,
in the metadata
add "showFilterBadge": false to the dashboard metadata
see that filter badges are hidden
use-case 2
0 . enable DASHBOARD_CROSS_FILTERS ff
add "showCrossFilterBadge": false to the dashboard metadata
see that cross filter badge is not shown for the echarts pie chart that configuredto emit filter events
use-case 3 :
play with combinations of showFilterBadge and showCrossFilterBadge and see that they act accordingly
ADDITIONAL INFORMATION