Conversation
Contributor
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
5 tasks
mbondyra
reviewed
May 12, 2020
mbondyra
reviewed
May 12, 2020
mbondyra
approved these changes
May 12, 2020
Contributor
mbondyra
left a comment
There was a problem hiding this comment.
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.
Contributor
|
@elasticmachine merge upstream |
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. |
crob611
approved these changes
May 13, 2020
Contributor
crob611
left a comment
There was a problem hiding this comment.
Canvas changes are good 👍
Contributor
Author
|
@elasticmachine merge upstream |
Contributor
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
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
This was referenced May 19, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
This PR changes it to:
for embedded lens visualizations
for in-editor len visualizations
It is using the
handlers.eventfunction passed to each expression renderer to dispatch an interpreter event which can be handled by the caller of theExpressionRendererComponent(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.
getTriggerinstead ofexecuteTriggerActionsThe ui_actions plugins exposes two different ways of dispatching actions
getTrigger(id).execandexecuteTriggerActions. 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#L277Passing 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:
In the interpreter event just the
datais 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#L270However in lens the renderer has to pass the timefield information as well. This is why this PR moves the
timeFieldNameintodataas well: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
LensFilterEventandLensBrushEvent: 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-73aa926dcfd650527803a9a900ab785eL35Making 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: