visualize embeddable to visualizations plugin #54840

ppisljar · 2020-01-14T22:50:44Z

Summary

moves visualize embeddable into visualizations plugin

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~[ ] Documentation was added for features that require explanation or tutorials~~
~~[ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2020-01-14T22:51:15Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

flash1293 · 2020-01-15T10:37:32Z

src/legacy/core_plugins/visualizations/public/embeddable/visualize_embeddable_factory.tsx

-import { start as visualizations } from '../../../visualizations/public/np_ready/public/legacy';
-import { showNewVisModal } from '../visualize';
-import { SavedVisualizations } from '../visualize/np_ready/types';
+import { showNewVisModal } from '../../../kibana/public/visualize/np_ready/wizard/show_new_vis';


This should also be exposed from the top level of the visualize folder, to avoid coupling visualizations to the folder structure of visualize

are you ok with leaving ti as is for now ? i tried top level import but that breaks some tests and i would like to keep this PR as small as possible.

in the follow up i will most likely move this to the visualizations plugin, as else we might end up with circular dependency (visualize_embeddable requires showNewVisModal, but kibana/visualize reqires a lot of things from visualization plugin)

Ah right, I have a PR open that should fix this problem: #54124

Then it's fine for now, moving it over makes sense to me as this isn't exclusively "visualize" UI

flash1293 · 2020-01-15T10:38:44Z

src/legacy/core_plugins/visualizations/public/np_ready/public/legacy.ts


-export const setup = pluginInstance.setup(npSetup.core);
-export const start = pluginInstance.start(npStart.core);
+export const setup = pluginInstance.setup(npSetup.core, {


nit: Could also be

export const setup = pluginInstance.setup(npSetup.core, npSetup.plugins); export const start = pluginInstance.start(npStart.core, npStart.plugins);

flash1293 · 2020-01-15T10:42:57Z

src/legacy/core_plugins/kibana/public/visualize/plugin.ts


-    const embeddableFactory = new VisualizeEmbeddableFactory(visualizations.types);
-    embeddables.registerEmbeddableFactory(VISUALIZE_EMBEDDABLE_TYPE, embeddableFactory);
+    const savedVisualizations = createSavedVisLoader({


@kertal Changes how this works in this PR and it looks like it will make this logic unnecessary: #54155

@ppisljar Can you wait with your PR till that one is merged? This PR introduces managementRegistry and uiModules in this file which I would like to avoid because Matthias' PR will get rid of it anyway.

seems that PR is reviewed and just waiting for 7.6 branch ?
i am fine with it merging first, this will probably anyway take another day to review

if it happens the other way around (and this is ready first) i also don't think it should be a problem, either @kertal removes the uiModules and managementRegistry from this file, or i can do it in followup

Sounds good to me, please leave a TODO comment to document it somewhere.

Better than a comment would be to pass in uiModules and managementRegistry as a __LEGACY plugin dependency, just to follow the convention of shimmed plugins.

I've moved #54155 to 7.7, it's no problem to wait until this is merged in

lizozom

Checked out and tested locally.
Added some minor comments and overall LGTM.

lizozom · 2020-01-15T11:05:50Z

src/legacy/core_plugins/visualizations/public/embeddable/get_index_pattern.ts


-  const savedObjectsClient = npStart.core.savedObjects.client;
-  const defaultIndex = npStart.core.uiSettings.get('defaultIndex');
+  const savedObjectsClient = getSavedObjectsClient().client;


Either getSavedObjectsClient should return the client, or its name should be getSavedObjects

src/legacy/core_plugins/visualizations/public/embeddable/visualize_embeddable_factory.tsx

src/legacy/core_plugins/visualizations/public/np_ready/public/services.ts

# Conflicts: # src/legacy/core_plugins/visualizations/public/embeddable/visualize_embeddable_factory.tsx

ppisljar · 2020-01-15T11:39:59Z

@elastic/kibana-design sass files were moved (no changes)

ppisljar · 2020-01-15T11:42:47Z

x-pack/legacy/plugins/canvas/canvas_plugin_src/expression_types/embeddable_types.ts

@elastic/kibana-canvas this import changed

ppisljar · 2020-01-15T11:42:58Z

x-pack/legacy/plugins/canvas/canvas_plugin_src/functions/common/saved_visualization.ts

@elastic/kibana-canvas this import changed

crob611

Canvas changes LGTM 👍

cchaos · 2020-01-15T15:05:21Z

src/legacy/core_plugins/kibana/public/index.scss


 // Visualize styles
 @import './visualize/index';
-@import './visualize_embeddable/index';


You seem to now be missing the import of this index file from inside the legacy folder. So the styles won't be compiled.

cchaos · 2020-01-15T16:00:49Z

src/legacy/core_plugins/kibana/public/index.scss

 // Visualize styles
 @import './visualize/index';
-@import './visualize_embeddable/index';
+@import '../../visualizations/public/embeddable/index';


Sorry I didn't mean you should change the import path in this file. You shouldn't be importing SASS files from other plugins. Each plugin needs their own manifest file index.scss. You can follow the pattern setup in this folder https://github.com/elastic/kibana/tree/c5306961d3ac4826ac044dc8d4b9031d8b0f4a8b/src/legacy/core_plugins/newsfeed/public

cchaos

Sass imports look good now

kibanamachine · 2020-01-15T18:53:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d072d27

History

💚 Build #20582 succeeded 6d41654
💚 Build #20525 succeeded c530696
💔 Build #20516 failed 1ff2905efc90f7758501a283f4083edb99ab75e1
💚 Build #20435 succeeded 01bd4f2
💔 Build #20415 failed 51dcb06

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

ppisljar added 2 commits January 14, 2020 15:57

moving visualize embeddable to visualizations plugin

b903be4

updating

9113a77

ppisljar requested a review from a team January 14, 2020 22:50

ppisljar requested review from a team as code owners January 14, 2020 22:50

ppisljar added release_note:skip Skip the PR/issue when compiling release notes review Team:AppArch v7.7.0 v8.0.0 labels Jan 14, 2020

ppisljar requested a review from lizozom January 14, 2020 22:52

ppisljar added 2 commits January 14, 2020 18:36

fixes

d8c3024

fixing imports

51dcb06

ppisljar force-pushed the vis/embeddable branch from 10c99e4 to 51dcb06 Compare January 15, 2020 00:35

fixing saved visualizations registration

01bd4f2

lizozom assigned ppisljar Jan 15, 2020

flash1293 reviewed Jan 15, 2020

View reviewed changes

lizozom approved these changes Jan 15, 2020

View reviewed changes

Merge branch 'master' into vis/embeddable

b4a4552

# Conflicts: # src/legacy/core_plugins/visualizations/public/embeddable/visualize_embeddable_factory.tsx

ppisljar commented Jan 15, 2020

View reviewed changes

updating based on review

c530696

ppisljar force-pushed the vis/embeddable branch from 1ff2905 to c530696 Compare January 15, 2020 12:00

crob611 approved these changes Jan 15, 2020

View reviewed changes

cchaos suggested changes Jan 15, 2020

View reviewed changes

fixing based on review

6d41654

cchaos reviewed Jan 15, 2020

View reviewed changes

sass index file

d072d27

cchaos approved these changes Jan 15, 2020

View reviewed changes

ppisljar merged commit 297aa7b into elastic:master Jan 15, 2020

ppisljar added a commit to ppisljar/kibana that referenced this pull request Jan 15, 2020

visualize embeddable to visualizations plugin (elastic#54840)

cf77df4

ppisljar mentioned this pull request Jan 15, 2020

[7.x] visualize embeddable to visualizations plugin (#54840) #54951

Merged

ppisljar added a commit that referenced this pull request Jan 15, 2020

visualize embeddable to visualizations plugin (#54840) (#54951)

bf9f3bf

jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020

visualize embeddable to visualizations plugin (elastic#54840)

43ca891

visualize embeddable to visualizations plugin #54840

visualize embeddable to visualizations plugin #54840

Uh oh!

Conversation

ppisljar commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jan 14, 2020

Uh oh!

flash1293 Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ppisljar Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kertal Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ppisljar commented Jan 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crob611 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 15, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ppisljar commented Jan 14, 2020 •

edited

Loading

flash1293 Jan 15, 2020 •

edited

Loading

ppisljar Jan 15, 2020 •

edited

Loading

kertal Jan 15, 2020 •

edited

Loading