-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[uiActions] Support emitting nested triggers and actions #70602
Conversation
@elasticmachine merge upstream |
Pinging @elastic/kibana-app-arch (Team:AppArch) |
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, but i suggest keeping two distinct actions which autoexecute instead of groupuing value_click and range_select triggers under one action.
src/plugins/data/public/actions/emit_apply_filter_trigger_action.ts
Outdated
Show resolved
Hide resolved
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.
- Like the addition of
shouldAutoExecute
. - Verified the example plugin was updated, with the nested action (didn't test).
- Didn't pull down and test, just a quick code review.
- Didn't look too deeply into UiActionsExecutionService.
- Agree with Peter's remarks about not combining the two triggers.
- Some public API docs look like they could use some more comments.
Overall, approach looks good. 👍
src/plugins/ui_actions/public/service/ui_actions_execution_service.ts
Outdated
Show resolved
Hide resolved
…iggers # Conflicts: # src/plugins/data/public/public.api.md
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, tested on Mac Chrome, I only think we should add unit tests for the new service.
src/plugins/data/public/actions/filters/create_filters_from_range_select.ts
Show resolved
Hide resolved
filterManager.addFilters(restOfFilters); | ||
if (timeRangeFilter) { | ||
esFilters.changeTimeFilter(timeFilter, timeRangeFilter); | ||
shouldAutoExecute: async () => 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.
Curious, can this be
shouldAutoExecute: async () => true, | |
autoExecute: true, |
? I.e. does it have to by async?
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.
No use case now, but I thought of having it as flexible as possible from that start (doesn't add much burden). Recently had to change getHref: () => string
to getHref: () => Promise<string>
src/plugins/ui_actions/public/context_menu/build_eui_context_menu_panels.tsx
Outdated
Show resolved
Hide resolved
Talked over Zoom, the new service has some integration tests covering it, so unit test can be skipped. |
💚 Build SucceededBuild metricsmiscellaneous assets size
page load bundle size
History
To update your PR or re-run it, just comment with: |
* master: [Form lib] Memoize form hook object and fix hook array deps (elastic#71237) [uiActions] Support emitting nested triggers and actions (elastic#70602) add short sleep before clicking Remove on sample data (elastic#71104) Fixed the beta badge layout. (elastic#71835) Restores task for downloading Chromium builds (elastic#71749) [logging] Format new platform json logging to ECS (elastic#71138) add policy details and update SO limit requests (elastic#71789) Convert vis_type_vega to Typescript (elastic#68915) [ML] Fix UI Actions context menu positioning for the Anomaly Swim Lane (elastic#71839)
* Introduce automatically executed actions * Introduce batching of emitted triggers to be execute on the macro task
Summary
closes #59162
Needed for:
Clean up In next PR:
Implementation (more in #59162):
Concept of
automatically executed actions
Such actions won't be batched into 1 context menu but will automatically execute. This could be useful for other reasons anyway (consider the situation of a HOVER trigger and wanting to attach it to an action that shows a tooltip and an action that shows a global threshold line - those are two actions that should be executed simultaneously, and not shown to the user in a context menu for them to choose from).
Introduce batching of emitted triggers to be execute on the next event loop.
An auto executed action can be attached to VALUE_CLICK_TRIGGER - ACTION_EMIT_APPLY_FILTER_TRIGGER. This checks to see if the data can be transformed into a Filter shape. If it does, it uiActionApi.emit(APPLY_FILTER_TRIGGER, filters). Because this is auto executed, it gets emitted before the event loop, should get batched up and then the context menu shown to the user should contain both actions attached to VALUE_CLICK_TRIGGER and actions attached to APPLY_FILTER_TRIGGER
Checklist
Delete any items that are not applicable to this PR.