-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[NP] Visualize #62294
Conversation
# 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
Pinging @elastic/kibana-app (Team:KibanaApp) |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this 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.
src/plugins/visualize/kibana.json
Outdated
"ui": true, | ||
"requiredPlugins": [ | ||
"data", | ||
"embeddable", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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!
There was a problem hiding this 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
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* 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 <[email protected]> # Conflicts: # .github/CODEOWNERS # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json
* 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 <[email protected]> # Conflicts: # .github/CODEOWNERS # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json
Summary
Move the
Visualize
plugin into new platform.The changes are:
kibana-app
ascodeowner
;visualize
namespace for translations, fix translations;KibanaContextProvider
(before dependencies were passed through the default editor context, but when moved into NP this appeared in a separate bundle and become unavailable);Share
button intop nav
when share plugin is turned off (the same was done in dashboard);visualizations
plugin includes@import 'components/index'
insrc/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