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

[NP] Visualize #62294

Merged
merged 20 commits into from
Apr 16, 2020
Merged

[NP] Visualize #62294

merged 20 commits into from
Apr 16, 2020

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Apr 2, 2020

Summary

Move the Visualize plugin into new platform.

The changes are:

  • set kibana-app as codeowner;
  • create visualize namespace for translations, fix translations;
  • wrap Timelion options into KibanaContextProvider (before dependencies were passed through the default editor context, but when moved into NP this appeared in a separate bundle and become unavailable);
  • disabled Share button in top nav when share plugin is turned off (the same was done in dashboard);
  • visualizations plugin includes @import 'components/index' in src/plugins/visualizations/public/index.scss to be consumed by other plugins, but have to left over the import here : https://github.com/elastic/kibana/pull/62294/files#diff-bc1ed535c452a3c114f6622450f0bf1e for passing karma browser tests,
    so they are currently included twice temporarily, but it shouldn't be a problem

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@flash1293 flash1293 mentioned this pull request Apr 2, 2020
69 tasks
# Conflicts:
#	src/legacy/core_plugins/kibana/index.js
#	src/legacy/core_plugins/kibana/public/kibana.js
#	src/legacy/core_plugins/kibana/public/visualize/_index.scss
#	src/legacy/core_plugins/kibana/public/visualize/legacy_imports.ts
#	src/plugins/dashboard/public/index.ts
#	src/plugins/vis_default_editor/public/default_editor.tsx
#	src/plugins/vis_default_editor/public/default_editor_controller.tsx
#	src/plugins/visualize/public/kibana_services.ts
#	src/plugins/visualize/public/plugin.ts
@sulemanof sulemanof added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0 labels Apr 8, 2020
@elasticmachine
Copy link
Contributor

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

@sulemanof sulemanof marked this pull request as ready for review April 9, 2020 10:13
@sulemanof sulemanof requested a review from a team April 9, 2020 10:13
@sulemanof sulemanof requested a review from a team as a code owner April 9, 2020 10:13
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.

Tested in Chrome and couldn't find any problems - LGTM 🎉

@@ -27,64 +27,45 @@ import {
CoreStart,
Plugin,
PluginInitializerContext,
SavedObjectsClientContract,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning up the plugin class as well! Long overdue.

@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

@sulemanof
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.

Tested in Chrome and LGTM, I left one comment about the embeddable dependency.

"ui": true,
"requiredPlugins": [
"data",
"embeddable",
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't think of this initially, but @timroes is making a good point about this dependency in the Lens PR: #62965

As visualize works just fine without embeddables, this should be an optional dependency and usages should be wrapped into an if() { block similar to share and home

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A funny thing, this showed up Visualize doesn't need the embeddable dependency at all. Seems it's just a useless left over.
So I just removed it in one of the last commit. Thanks for this catch!

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

# Conflicts:
#	src/plugins/visualize/public/application/editor/editor.js
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@sulemanof sulemanof merged commit 6bbcabc into elastic:master Apr 16, 2020
@sulemanof sulemanof deleted the np/visualize branch April 16, 2020 15:32
sulemanof added a commit to sulemanof/kibana that referenced this pull request Apr 17, 2020
* Move visualize plugin to np

* Refactor plugin services

* Clean up

* Remove legacy style usage

* Fix style imports

* Fix timelion_options context provider

* Fix translations

* Change codeowners for visualize

* Import styles in legacy for BWC in Browser tests

* Get rid of embeddable dependency

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	.github/CODEOWNERS
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
sulemanof added a commit to sulemanof/kibana that referenced this pull request Apr 17, 2020
* Move visualize plugin to np

* Refactor plugin services

* Clean up

* Remove legacy style usage

* Fix style imports

* Fix timelion_options context provider

* Fix translations

* Change codeowners for visualize

* Import styles in legacy for BWC in Browser tests

* Get rid of embeddable dependency

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	.github/CODEOWNERS
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
sulemanof added a commit to sulemanof/kibana that referenced this pull request Apr 17, 2020
* Move visualize plugin to np

* Refactor plugin services

* Clean up

* Remove legacy style usage

* Fix style imports

* Fix timelion_options context provider

* Fix translations

* Change codeowners for visualize

* Import styles in legacy for BWC in Browser tests

* Get rid of embeddable dependency

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	.github/CODEOWNERS
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
sulemanof added a commit that referenced this pull request Apr 17, 2020
* Move visualize plugin to np

* Refactor plugin services

* Clean up

* Remove legacy style usage

* Fix style imports

* Fix timelion_options context provider

* Fix translations

* Change codeowners for visualize

* Import styles in legacy for BWC in Browser tests

* Get rid of embeddable dependency

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	.github/CODEOWNERS
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants