-
Notifications
You must be signed in to change notification settings - Fork 8.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
Use "Apply_filter_trigger" in "explore underlying data" action #71445
Use "Apply_filter_trigger" in "explore underlying data" action #71445
Conversation
76c3867
to
313e546
Compare
Dear @elastic/kibana-gis, Recently app-arch team introduced explore underlying data action. Before this pr that action was attached to low level "value_click" and "select_range" triggers, but this pr also attaches it to higher level "apply_filter" trigger. Since maps use "apply_filter" trigger directly, this new action didn't appear for maps before. @elastic/kibana-gis, do you think we should leave it enabled for maps? Or should I disable it? |
Pinging @elastic/kibana-app-arch (Team:AppArch) |
There are a couple of cases where "explore underlying data" provides an awkward user experience as is.
|
…igger-underlying-data
…igger-underlying-data
Thanks @nreese for checking this out.
Maybe you've checked that not inside map embeddable? (this functionality relies on embeddable and index pattern in embeddable output) |
For spatial filtering it really makes sense to disable this feature when the filter is added. I think the effort is worth looking into for better usability |
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 for purposes of this PR we should disable this action for Maps, and address it separately. Othwerwise, LGTM, checked on Ubuntu/Chrome.
…igger-underlying-data # Conflicts: # x-pack/plugins/discover_enhanced/public/actions/explore_data/abstract_explore_data_action.ts # x-pack/plugins/discover_enhanced/public/actions/explore_data/explore_data_chart_action.test.ts # x-pack/plugins/discover_enhanced/public/actions/explore_data/explore_data_context_menu_action.test.ts
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
Tested in Chrome and Firefox, still works fine for me - filters and ranges get carried over correctly. Not super familiar with the code, but Kibana app changes look sensible to me (I like how we don't need the embeddable service anymore)
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededBuild metricspage load bundle size
History
To update your PR or re-run it, just comment with: |
…ic#71445) * use apply filter trigger for “expore underlying data” * disable for maps for now Co-authored-by: Elastic Machine <[email protected]>
… (#74045) * use apply filter trigger for “expore underlying data” * disable for maps for now Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
* master: (39 commits) [Canvas][tech-debt] Rename __examples__ to __stories__ (elastic#73853) [Canvas] Storybook Redux Addon (elastic#73227) Use "Apply_filter_trigger" in "explore underlying data" action (elastic#71445) [maps] convert top nav config to TS (elastic#73851) [maps] fix fit to bounds for ES document layers with joins (elastic#73985) [Canvas][tech-debt] Refactor Toolbar (completes Kill Recompose.pure) (elastic#73309) [CI] In-progress Slack notifications (elastic#74012) [SIEM][Detection Engine] Fixes tags to accept characters such as AND, OR, (, ), ", * (elastic#74003) [SECURITY_SOLUTION][ENDPOINT] Fix host list Configuration Status cell link loosing list page/size state (elastic#73989) Tweak injected metadata (elastic#73990) Closes elastic#73998 by using `canAccessML` in the ML capabilities API to (elastic#73999) [SIEM] Fixes toaster errors when siemDefault index is an empty or empty spaces (elastic#73991) [Security Solution] Fix timeline pin event callback (elastic#73981) [Security Solution] Fix unexpected redirect (elastic#73969) [Metrics UI] Fix Metrics Explorer TSVB link to use workaround pattern (elastic#73986) [APM] docs: Update machine learning integration (elastic#73597) [Ingest Manager] Fix limited concurrency helper (elastic#73976) [build/sysv] fix missing env variable rename (elastic#73977) Fix a typo. (elastic#73948) [Ingest Manager] Revert fleet config concurrency rollout to rate limit (elastic#73940) ...
Summary
Small clean up made possible by #70602
Refactor "explore underlying data" action to use apply filter directly
I didn't expect it would also enable this feature for maps 😅
see: #71445 (comment)
For now I disabled it to move forward with this small refactoring because of UX considerations with spatial filter.
Created to follow up: #73043
Checklist
Delete any items that are not applicable to this PR.