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] Navigate from discover to lens #77873

Merged
merged 25 commits into from
Oct 6, 2020

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Sep 18, 2020

Summary

Closes #67415.

As we want to promote Lens, this PR changes the navigation from Discover (Visualize button) to Lens instead of the legacy visualizations. On OSS nothing changes, this applies only for Basic+ licenses.

To accomplish this, Lens must unregister the Visualize action and register its own. It adds the indexPatternId and the fieldName to the location state and passes it to the Lens architecture in order to depict the suggestions to the users.

Note: We decided to not have a link for Lens right now, for this reason the right click on Visualize button doesn't work anymore.

Example of bytes field (kibana_sample_data_logs dataset) visualization to Lens:
Screenshot 2020-09-24 at 3 14 29 PM

Checklist

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula added Feature:Lens v7.11.0 v8.0.0 enhancement New value added to drive a business result release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Sep 24, 2020
@stratoula stratoula marked this pull request as ready for review September 24, 2020 13:35
@stratoula stratoula requested a review from a team September 24, 2020 13:35
@stratoula stratoula requested a review from a team as a code owner September 24, 2020 13:35
@elasticmachine
Copy link
Contributor

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

@stratoula stratoula requested a review from flash1293 September 24, 2020 13:35
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look at this! I would mainly like you to make this more generic, with the perspective that we want to add other ways of navigating to Lens from apps, not just discover.

export type ActionType = keyof ActionContextMapping;

export interface ActionContextMapping {
[DEFAULT_ACTION]: BaseContext;
[ACTION_VISUALIZE_FIELD]: VisualizeFieldContext;
[ACTION_VISUALIZE_GEO_FIELD]: VisualizeFieldContext;
[ACTION_VISUALIZE_LENS_FIELD]: VisualizeFieldContext;
Copy link
Contributor

Choose a reason for hiding this comment

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

So if I understand this correctly, this is using the same trigger, but different action for Lens- this sounds like what we agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly same trigger VISUALIZE_FIELD_TRIGGER but different action!

x-pack/plugins/lens/kibana.json Show resolved Hide resolved
x-pack/plugins/lens/public/app_plugin/app.tsx Outdated Show resolved Hide resolved
x-pack/plugins/lens/public/app_plugin/app.tsx Outdated Show resolved Hide resolved
x-pack/plugins/lens/public/app_plugin/app.tsx Outdated Show resolved Hide resolved
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Didn't test, uiActions usage lgtm

x-pack/plugins/lens/public/plugin.ts Outdated Show resolved Hide resolved
src/plugins/ui_actions/public/types.ts Show resolved Hide resolved
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Great work so far @stratoula ! I added some comments, but it feels really close already.

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Almost perfect from my side, left a small comment

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Thanks, Lens changes LGTM once build is green

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula requested a review from wylieconlon October 5, 2020 06:14
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
lens 563 564 +1

async chunks size

id before after diff
lens 1.1MB 1.1MB +3.6KB

page load bundle size

id before after diff
lens 76.0KB 78.7KB +2.7KB
uiActions 80.6KB 80.8KB +184.0B
visualize 41.4KB 41.4KB +47.0B
total +2.9KB

History

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

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Reviewed and tested again, LGTM!

@@ -132,6 +134,11 @@ export async function mountApp(
onAppLeave={params.onAppLeave}
setHeaderActionMenu={params.setHeaderActionMenu}
history={routeProps.history}
initialContext={
historyLocationState && historyLocationState.type === ACTION_VISUALIZE_LENS_FIELD
? historyLocationState.payload
Copy link
Contributor

Choose a reason for hiding this comment

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

This is okay, but I was almost expecting the entire historyLocationState to be passed in here so that the app is making fewer decisions. No need to change it unless you're planning on making other changes.

@stratoula stratoula added v7.10.0 and removed v7.11.0 labels Oct 6, 2020
@stratoula stratoula merged commit 08a4586 into elastic:master Oct 6, 2020
stratoula added a commit to stratoula/kibana that referenced this pull request Oct 6, 2020
* Create lens action and unregister the visualize one

* remove console

* Implement Discover to Lens, wip, missing tests

* Add unit tests

* fix embed lens to empty dashboard functional tests

* fix suggestions on save

* Fix bug on save button, query and filters should be transferred from discover

* Add functional test for the navigation from Discover to Lens

* PR update after code review

* unregister visualize action only if the action exists

* Change the test to not be flaky

* Move suggestions to editor frame and hide the emptyWorkspace for visualize field

* Update ui actions docs

* Add a retry to remove test flakiness

* Fix bug of infinite loader when removing the y axis dimension

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.actioncontextmapping.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.addtriggeraction.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.getaction.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.gettriggeractions.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.gettriggercompatibleactions.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.registeraction.md
#	src/plugins/ui_actions/public/public.api.md
stratoula added a commit that referenced this pull request Oct 6, 2020
* Create lens action and unregister the visualize one

* remove console

* Implement Discover to Lens, wip, missing tests

* Add unit tests

* fix embed lens to empty dashboard functional tests

* fix suggestions on save

* Fix bug on save button, query and filters should be transferred from discover

* Add functional test for the navigation from Discover to Lens

* PR update after code review

* unregister visualize action only if the action exists

* Change the test to not be flaky

* Move suggestions to editor frame and hide the emptyWorkspace for visualize field

* Update ui actions docs

* Add a retry to remove test flakiness

* Fix bug of infinite loader when removing the y axis dimension

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.actioncontextmapping.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.addtriggeraction.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.getaction.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.gettriggeractions.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.gettriggercompatibleactions.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.md
#	docs/development/plugins/ui_actions/public/kibana-plugin-plugins-ui_actions-public.uiactionsservice.registeraction.md
#	src/plugins/ui_actions/public/public.api.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Visualizations triggered by Discover should be handled by Lens
6 participants