Skip to content
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] pass trigger into action execution context #74363

Merged
merged 23 commits into from
Aug 14, 2020

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Aug 5, 2020

Summary

Partially addresses: #63854
This is a follow up on #71675 which takes a bit different (more strict) approach.

Reason why I didn't like #71675 is that it was super easy to not pass trigger into action handler.
I even missed some places in that pr.

This pr takes different approach:

We already have a separation between Action and ActionDefinition.
Those are used almost interchangeably in different places, which is confusing. The usage should be aligned in #74501:

  1. Action should be used internally and should represent the object returned by uiActions service
  2. ActionDefinition is a simple interface and should be used for registering an action in uiAction service.

With that:

As ActionDefinition should stay as simple and flexible as possible to provide the simplest api for defining and action, argument type of handlers do not required to specify trigger.
So when defining an action as ActionDefinition usage stays the same in simple cases:

class MyAction implements ActionDefinition<Context> {
....
execute(context: Context) {

}
....
}

But if need to access context, consumer could use type helper:

class MyAction implements ActionDefinition<Context> {
....
execute(context: ActionExecutionContext <Context>) {
  // to access the trigger:
  // console.log(context.trigger)
}
....
}

But in Action interface trigger is required now.
So if consumer uses action directly or defines an action using Action interface instead of ActionDefinition then:

action.exec({...context, trigger})

Action, ActionDefinition, createAction are used interchangeably and intersect with each other. These interface usage should be documented and aligned: #74501

@Dosant Dosant requested a review from streamich August 5, 2020 13:02
@Dosant
Copy link
Contributor Author

Dosant commented Aug 5, 2020

@elasticmachine merge upstream

1 similar comment
@Dosant
Copy link
Contributor Author

Dosant commented Aug 6, 2020

@elasticmachine merge upstream

@Dosant Dosant changed the title Dev/ui actions improve context 2 [UiActions] pass trigger into action execution context Aug 10, 2020
@Dosant Dosant added Feature:UIActions UI actions. These are client side only, not related to the server side actions.. Team:AppArch labels Aug 10, 2020
@Dosant Dosant added the v7.10.0 label Aug 10, 2020
@Dosant Dosant added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 10, 2020
Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Chrome, LGTM.

@Dosant
Copy link
Contributor Author

Dosant commented Aug 13, 2020

@elasticmachine merge upstream

@Dosant Dosant marked this pull request as ready for review August 13, 2020 12:15
@Dosant Dosant requested a review from a team as a code owner August 13, 2020 12:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@Dosant
Copy link
Contributor Author

Dosant commented Aug 14, 2020

@elasticmachine merge upstream

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Aug 14, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
uiActions 32 +1 31

page load bundle size

id value diff baseline
embeddable 428.6KB +1.4KB 427.2KB
uiActions 213.6KB +3.4KB 210.2KB
total +4.8KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@Dosant Dosant merged commit 7bd014a into elastic:master Aug 14, 2020
Dosant added a commit to Dosant/kibana that referenced this pull request Aug 14, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Dosant added a commit that referenced this pull request Aug 14, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 14, 2020
* master:
  Fix bug on TopN weird behavior with zero values (elastic#74942)
  [Lens] Fix table sorting bug (elastic#74902)
  [SECURITY_SOLUTION] Retry on ingest setup (elastic#75000)
  [file upload] lazy load to reduce page load size (elastic#74967)
  Drilldowns for TSVB / Vega / Timelion (elastic#74848)
  [EventLog] Populate alert instances view with event log data (elastic#68437)
  [UiActions] pass trigger into action execution context (elastic#74363)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embedding Embedding content via iFrame Feature:UIActions UI actions. These are client side only, not related to the server side actions.. release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants