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

Lens drilldowns #65675

Merged
merged 16 commits into from
May 15, 2020
Merged

Lens drilldowns #65675

merged 16 commits into from
May 15, 2020

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented May 7, 2020

This PR makes drilldowns work on lens visualizations (except for metric vis).

Description

Currently drilldowns don't work with Lens visualizations, because the action context dispatched by the Lens visualization doesn't contain the embeddable. This is due to the fact that in Lens, the renderer which dispatches the action is oblivious of its context (not reference of embeddables available).

Currently the control flow is like this:

elastic-charts event callback -> lens renderer -> ui_actions contract

This PR changes it to:

for embedded lens visualizations

elastic-charts event callback -> lens renderer -> interpreter event callback -> lens embeddable -> ui_actions contract

for in-editor len visualizations

elastic-charts event callback -> lens renderer -> interpreter event callback -> workspace panel -> ui_actions contract

It is using the handlers.event function passed to each expression renderer to dispatch an interpreter event which can be handled by the caller of the ExpressionRendererComponent (the lens embeddable respectively the workspace panel when working in the lens app).

By getting the embeddable into the control flow, it can add a reference to its own instance to the context before actually dispatching the action.

Related changes

This PR changes a few other things as well because they were close to the things I already edited.

getTrigger instead of executeTriggerActions

The ui_actions plugins exposes two different ways of dispatching actions getTrigger(id).exec and executeTriggerActions. The latter one will be removed in the future, this is why this PR is getting rid of it now. It is done the same way in visualize: https://github.com/elastic/kibana/blob/master/src/plugins/visualizations/public/embeddable/visualize_embeddable.ts#L277

Passing down contracts instead of "services module" pattern

This PR removes the "services" module currently used to get the ui_actions contract to the right places in favor of passing the reference down explicitly. This was done because it has a bunch of inherent advantages and due to this change the usage of the contract was moved higher up the tree so it wasn't necessary to pass it through a lot of layers.

https://github.com/elastic/kibana/pull/65675/files#diff-d0e6bfecc21668b38eb61133e950e3beR290
https://github.com/elastic/kibana/pull/65675/files#diff-01b9bb6d5671b345411620a933fe3166R122
https://github.com/elastic/kibana/pull/65675/files#diff-c58ac0c13efb1ab97edbc0a8edc5c490

Changing trigger context shape

Currently the trigger context looks like this:

{
  embeddable: IEmbeddable;
  data: { /* different things depending on type */ };
  timeFieldName?: string;
}

In the interpreter event just the data is passed up from the chart itself which works fine in visualize because the visualize embeddable adds the timefield before dispatching the action: https://github.com/elastic/kibana/blob/master/src/plugins/visualizations/public/embeddable/visualize_embeddable.ts#L270

However in lens the renderer has to pass the timefield information as well. This is why this PR moves the timeFieldName into data as well:

{
  embeddable: IEmbeddable;
  data: { /* different things depending on type */   timeFieldName?: string };
}

This makes it possible to use the same event interface in lens and in visualize, making it easier to later have unified types.

Remove Trigger type references in Lens code

Previously to this change, the lens renderers had knowledge about the ui_actions plugin and triggers. As this PR moves this responsibility to the workspace panel and the embeddable implementation, the renderers don't need that information anymore - they just have to conform to the interface of the event reducing the coupling to a single place.

This is why this PR introduces the notion of the LensFilterEvent and LensBrushEvent: https://github.com/elastic/kibana/pull/65675/files#diff-c9e959476594f76ae1ce129e8beb7e50R481 (those can become a generic type for expression events in a later PR). In the renderers themselfes, only these types are referenced: https://github.com/elastic/kibana/pull/65675/files#diff-73aa926dcfd650527803a9a900ab785eL35

Making uiActions optional

Lens charts work just fine when actions/drilldowns are not available. This PR makes the dependency optional https://github.com/elastic/kibana/pull/65675/files#diff-a919c2b08d3aa3c9382511af12f1d499L15 and just doesn't dispatch actions if the contract is not available.

Removing lens exceptions in drilldowns and canvas

As lens is now behaving according to the ui_actions spec, the workarounds in drilldown and canvas code could be removed:

@botelastic botelastic bot added Feature:Drilldowns Embeddable panel Drilldowns Feature:Embedding Embedding content via iFrame labels May 7, 2020
@flash1293 flash1293 added Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure labels May 8, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@flash1293 flash1293 marked this pull request as ready for review May 8, 2020 08:10
@flash1293 flash1293 requested a review from a team May 8, 2020 08:10
@flash1293 flash1293 requested review from a team as code owners May 8, 2020 08:10
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.

AppArch changes LGTM.

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

I love the description of this PR, thanks for putting an effort into explaining what you've changed so clearly and with all the details!
The code looks good. Tested on Firefox and Chrome. Didn't test Canvas part.

@mbondyra
Copy link
Contributor

@elasticmachine merge upstream

@flash1293
Copy link
Contributor Author

@elastic/kibana-canvas Can someone approve from the canvas side? This PR removes the workaround which is currently necessary to prevent ui actions from being dispatched by embedded lens visualizations.

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Canvas changes are good 👍

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@flash1293 flash1293 merged commit 5207009 into elastic:master May 15, 2020
@flash1293 flash1293 deleted the lens-drilldowns branch May 15, 2020 10:01
flash1293 added a commit to flash1293/kibana that referenced this pull request May 15, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 15, 2020
…ent/add-support-in-url-for-hidden-toggle

* 'master' of github.com:elastic/kibana: (34 commits)
  [SIEM][CASE] Fix bug when connector is deleted. (elastic#65876)
  [SIEM][CASE] Improve layout (elastic#66232)
  [Index Management] Support Hidden Indices (elastic#66422)
  Add Login Selector functional tests. (elastic#65705)
  Lens drilldowns (elastic#65675)
  [ML] Custom template for apiDoc markdown (elastic#66567)
  Don't bootstrap core type emits (elastic#66377)
  [Dashboard] Improve loading error handling (elastic#66372)
  [APM] Minor style fixes for the node strokes (elastic#66574)
  [Ingest Manager] Fix create data source from integration (elastic#66626)
  [Metrics UI] Fix default metric alert interval for new conditions (elastic#66610)
  [Metrics UI] Fix alignment and allow clearing metric value (elastic#66589)
  Don't return package name for non-package data streams (elastic#66606)
  [Ingest Manager] Consolidate routing and add breadcrumbs to all pages (elastic#66475)
  [Docs/Reporting] Have the docs about granular timeout match Cloud docs (elastic#66267)
  Don't automatically add license header to code inside plugins dir. (elastic#66601)
  [APM] Don't trigger map layout if no elements (elastic#66625)
  [Logs UI] Validate ML job setup time ranges (elastic#66426)
  Fix pagination bugs in CCR and Remote Clusters (elastic#65931)
  Add cloud icon for supported settings and embed single-sourced getting started (elastic#65610)
  ...

# Conflicts:
#	x-pack/plugins/index_management/public/application/sections/home/index_list/index_table/index_table.js
#	x-pack/plugins/index_management/server/lib/fetch_indices.ts
flash1293 added a commit that referenced this pull request May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Drilldowns Embeddable panel Drilldowns Feature:Embedding Embedding content via iFrame Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants