-
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
[Visualizations] Enables multiple values filtering on tooltip actions #148372
Conversation
: undefined | ||
} | ||
actions={ | ||
!args.detailedTooltip |
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 am enabling this only for the EC tooltip and not the custom detailed one (it is only used for agg based)
src/plugins/chart_expressions/expression_xy/public/components/xy_chart.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/xy_chart.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/xy_chart.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_xy/public/components/xy_chart.tsx
Outdated
Show resolved
Hide resolved
Waiting for the #144601 to unblock this. |
Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations) |
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.
Nice addition! LGTM 👍
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.
Presentation team trigger addition LGTM!
Just out of curiosity, in the example gif in the PR description, why is the resulting filter set up to be an OR
, when it might make more sense as an is one of
filter, because all of the selections are from the same field?
@ThomThomson it would be more complicated (indentifying the cases where it would work or not), using only the combined filter can cover all cases (and make our code much simpler). We could do it as a second step of course. |
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.
Done a code review. Left some minor comments.
src/plugins/chart_expressions/expression_xy/public/components/xy_chart.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data/public/actions/filters/create_filters_from_multi_value_click.ts
Outdated
Show resolved
Hide resolved
{ | ||
id: dataViewId, | ||
} as DataView, |
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 we can consider also changing the type signature of buildCombinedFilter
to
indexPattern: Pick<DataViewBase, 'id'>,
so you don't have to cast to a DataView
or to a DataViewBase
because the buildCombinedFilter
only use the indexPattern id
property internally
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.
Done ab3831e
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: |
Summary
Enables tooltip actions on:
I am not enabling this in TSVB.
Also the action is not enabled for Lens if:
Checklist