-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
feat(dashboard): menu improvements, fallback support for Drill to Detail #21351
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21351 +/- ##
==========================================
- Coverage 66.84% 66.81% -0.03%
==========================================
Files 1799 1800 +1
Lines 68902 68943 +41
Branches 7324 7337 +13
==========================================
+ Hits 46057 46067 +10
- Misses 20955 20985 +30
- Partials 1890 1891 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
left a quick suggestion
1e9411d
to
de7d37e
Compare
de7d37e
to
63a5626
Compare
8095699
to
a3cbd47
Compare
a3cbd47
to
3cdf7b9
Compare
add2f40
to
4b6c276
Compare
Thanks for the awesome PR description @codyml. I have a suggestion while looking at the screenshots. Can we standardize the way we display messages? For example: I like disabling the option and showing an icon with a tooltip. Can we do the same for the "Drill to detail by" option? As you can see in the screenshot below, the message is displayed as a sub-item and the text is greyed out which is hard to read. It also forces the user to open the sub-menu before realizing the operation is not supported. I think using the icon plus tooltip also helps when the user already knows the possible causes and just wants to see if the option is enabled or not. |
@michael-s-molina that works for me! @kasiazjc any thoughts? |
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
This PR does some code cleanup, adds/updates testing, and extends support for Drill to Detail to additional edge cases.
SliceHeaderControls
andChart
components. This PR combines/moves all Drill to Detail code toChart/DrillDetail
.contextmenu
event. This PR adds a fallbackcontextmenu
event handler to theChartRenderer
chart wrapper that allows access to Drill to Detail without any filters, just like what's currently possible from the chart header menu.contextmenu
event: right-click menu has enabled "Drill to detail" and enabled "Drill to detail by" submenu containing just a message noting the lack of support. Header dropdown menu has enabled "Drill to detail".Notes for adding/improving Drill to Detail support for viz plugins
contextmenu
event such that theonContextMenu
chart hook gets called with filters corresponding to whatever dimension(s) the user right-clicked on.Behavior.DRILL_TO_DETAIL
behavior to the plugin'sChartMetadata
instance. The Drill to Detail menu component checks for this behavior wheneveronContextMenu
is called without filters to determine if it should show the "not yet supported" message or the "you didn't select a dimension" message. Therefore, if you add theBehavior.DRILL_TO_DETAIL
behavior, you can let the fallback handler catch right-clicks to parts of the chart that don't correspond to dimension values and the menu will correctly interpret these clicks as clicks not corresponding to a dimension rather than clicks to an unsupported chart.noAggregations
function to the viz plugin'sChartMetadata
instance that receivesformData
as a param and returnstrue
if the chart contains only atomic data.cc: @kasiazjc @michael-s-molina
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
Screen.Recording.2022-10-05.at.6.24.25.PM.mov
After
Screen.Recording.2022-10-05.at.6.21.43.PM.mov
TESTING INSTRUCTIONS
DRILL_TO_DETAIL
ADDITIONAL INFORMATION
DRILL_TO_DETAIL