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

[Discuss] Trigger context interface #63854

Closed
streamich opened this issue Apr 17, 2020 · 2 comments
Closed

[Discuss] Trigger context interface #63854

streamich opened this issue Apr 17, 2020 · 2 comments
Labels
Feature:UIActions UI actions. These are client side only, not related to the server side actions.. impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort

Comments

@streamich
Copy link
Contributor

streamich commented Apr 17, 2020

Currently user passes context object to trigger.exec() method and that object is passed to actions as-is. I would like to propose to augment data passed to trigger.exec() and pass to actions an augmented/standartize Context object.

Now user passes context directly:

const context = { embeddable };
plugins.uiActions.getTrigger(...).exec(context)

Instead user would pass in data and augmented context would be created by ui_actions internally:

const data = { embeddable };
plugins.uiActions.getTrigger().exec({ data });

Initial implementation of Context class would have user supplied data and two extra fields: trigger and disabledActions.

class Context<Data> {
  public readonly data: Data;

  // Trigger which cased the action to execute. Useful to report it, because an
  // action can be attached to multiple triggers.
  public readonly trigger: Trigger;

  // A list of action that should be disabled (usually in embeddable). Having this
  // list on `Context` would allow `ActionInternal` automatically disable all 
  // `disabledActions` by automatically enhancing the `.isCompatible` method
  // of `ActionDefinition`.
  public readonly disabledActions: string[];

  constructor(trigger: Trigger, params: { data: Data; disabledActions?: string[] }) {
    this.trigger = trigger;
    this.data = data;
    this.disabledActions = disabledActions;
  } 
}

class Trigger {
  exec({ data, disabledActiosn? }) {
    const context = new Context(this, { data, disabledActions });
    this.uiActionsService.executeActions(context);
  }
}
  • Having trigger on Context interface would be useful because an action can be attached to multiple triggers, this would let action know which trigger executed it.

  • Having disabledActions field on Context would allow us to move disabledActions logic out of embeddable panels, which currently uses disabledActions only for three triggers CONTEXT_MENU_TRIGGER, PANEL_BADGE_TRIGGER, and PANEL_NOTIFICATION_TRIGGER. Having moved disabledActions out of the panel would make this flag work everywhere in ui_actions, not just embeddable panel, and not just for the above 3 triggers.

  • Having Context class would allow us in the future add more features to it. An embeddable specific context could have:

    context.isEmbeddableAContainer()
    context.isEmbeddableEditable
    context.embeddableEditUrl

Alternative design could be to allow user to create the context object themselves.

const context = new Context('VALUE_CLICK_TRIGGER', { embeddable, disabledActions });

And make custom data available immediately on context. object instead of context.data..

context.embeddable
// instead of
context.data.embeddable

This would allow users to create their contexts using inheritance.

class VisEmbeddableContext extends Context {
  constructor(embeddable, filters) {
    super('VALUE_CLICK_TRIGGER', { embeddable, filters });
  }
}

P.S. The idea is basically taken from DOM Event API and CustomEvent, where browser's Event has predefined functionality, like .preventDefault(), .bubble etc. and you can specify custom "data" when creating a browser event.

image

cc @stacey-gammon @elastic/kibana-app-arch

Part of #71854

@streamich streamich added Feature:UIActions UI actions. These are client side only, not related to the server side actions.. Team:AppArch labels Apr 17, 2020
This was referenced Jul 1, 2020
@streamich streamich mentioned this issue Jul 15, 2020
26 tasks
@Dosant Dosant added the loe:medium Medium Level of Effort label Jul 21, 2020
@Dosant
Copy link
Contributor

Dosant commented Sep 8, 2020

@streamich, as you know we implemented #74363 which solved immediate problem and allowed us to passed trigger with action context. This worked out fine for URL drilldown.
We didn't follow the path proposed in this issue, as at that moment we agreed that it would be a significant change in bunch of places.

Do you think we could close this issue and as we need add more props (like disabledActions) we would just follow similar approach, or would you like to still leave it open in case we want to revise current solution?

@exalate-issue-sync exalate-issue-sync bot added the impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. label Jun 21, 2021
@vadimkibana
Copy link
Contributor

Thank you for contributing to this issue, however, we are closing this issue due to inactivity as part of a backlog grooming effort. If you believe this feature/bug should still be considered, please reopen with a comment.

@vadimkibana vadimkibana closed this as not planned Won't fix, can't repro, duplicate, stale Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:UIActions UI actions. These are client side only, not related to the server side actions.. impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort
Projects
None yet
Development

No branches or pull requests

3 participants