Move new_vis_modal to visualizations plugin by Dosant · Pull Request #56654 · elastic/kibana

Dosant · 2020-02-03T17:25:03Z

Summary

Parent issue: #46809
Move new_vis_modal to visualizations plugin.
These removes one import from visualisations plugin to kibana/

Dev Docs

Before

NewVisModal component and showNewVisModal function were statically exported and received all the dependencies as props/parameters.

After

showNewVisModal() is part of the plugin contract and plugin dependencies are provided implicitly.

npStart.plugins.visualizations.showNewVisModal();

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

…/show-new-vis-modal

elasticmachine · 2020-02-04T10:45:53Z

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

flash1293

Code LGTM, didn't pull down to test

flash1293 · 2020-02-04T10:59:10Z

src/legacy/core_plugins/visualizations/public/np_ready/public/wizard/new_vis_modal.tsx

-const baseUrl = `#${VisualizeConstants.CREATE_PATH}?`;
+// TODO: redirect logic is specific to visualise & dashboard
+// but it is likely should be decoupled. e.g. handled by the container instead
+const baseUrl = `#/visualize/create?`;


+1, can you create an issue for that?

ppisljar · 2020-02-04T11:29:30Z

src/legacy/core_plugins/visualizations/public/np_ready/public/wizard/show_new_vis.tsx

 import { IUiSettingsClient, SavedObjectsStart } from 'kibana/public';
 import { NewVisModal } from './new_vis_modal';
-import { TypesStart } from '../../../../../visualizations/public/np_ready/public/types';
+import { TypesStart } from '../types';


currenty NewVisModal component and showNewVisModal function are statically exported and receive all dependencies as props/parameters.

i think it makes sense to rather export them on the contract so most of the parameters can be provided and plugin setup. calling showNewVisModal should be as simple as:

npStart.plugins.visualizations.showNewVisModal();

I ended up with:

expose showNewVisModal() on vis plugin - as you suggested. But in vis embeddable factory I am still using it as function - https://github.com/elastic/kibana/pull/56654/files#diff-a45935fc131c617ca6ad658838e474ebR40. As it is part of the same plugin. Not sure if this is correct?

I started using it in visualise_listing and removed custom angular directive wrapper around NewVisModal react component

1. expose showNewVisModal() on vis plugin 2. use it in visualise instead of custom directive

ppisljar

code LGTM

Dosant · 2020-02-05T10:33:15Z

@elastic/kibana-design, could you take a look please? Just moving .scss

cchaos · 2020-02-05T14:29:26Z

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

 @import 'src/legacy/ui/public/styles/styling_constants';

 @import './embeddable/_index';
+@import './np_ready/public/wizard/_index';


Really the np_ready folder should have it's own _index.scss file as well, but this is ok since I know there's still a lot of file shuffling happening.

Side note, with SCSS files, you don't need to include the underscore _ in the import file names. Ex:

Suggested change

@import './np_ready/public/wizard/_index';

@import './np_ready/public/wizard/index';

Thanks, improved

cchaos · 2020-02-05T15:43:43Z

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


-@import './embeddable/_index';
+@import './embeddable';
+@import './np_ready/public';


Sorry, I think there was some confusion, but you still need to explicitly write out the index portion of the import. SASS doesn't automatically look for an index file.

Suggested change

@import './np_ready/public';

@import './np_ready/public/index';

Thank you. was confused by still seeing everything working locally. Looks like cache.

cchaos

Better, thanks!

Dosant · 2020-02-06T08:01:56Z

@elasticmachine merge upstream

kibanamachine · 2020-02-06T09:23:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9adda44

History

💚 Build #24667 succeeded 8bc6f8e
💚 Build #24306 succeeded a8ec165
💔 Build #24290 failed 9dc0b46
💚 Build #24264 succeeded 9342bef
💔 Build #24088 failed 2efa6b0

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

Move new_vis_modal to visualizations plugin. These removes one import from visualisations plugin to kibana/ showNewVisModal() is part of the plugin contract and plugin dependencies are provided implicitly. # Conflicts: # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json

Dosant added 3 commits February 3, 2020 18:19

wip

2efa6b0

fix i18n

1392742

Merge branch 'master' of github.com:elastic/kibana into dev/vis-to-np…

3371af9

…/show-new-vis-modal

streamich mentioned this pull request Feb 4, 2020

Move functionality from kibana to visualizations #46809

Closed

4 tasks

Dosant added Feature:NP Migration Team:AppArch labels Feb 4, 2020

Dosant added v8.0.0 v7.7.0 review release_note:skip Skip the PR/issue when compiling release notes labels Feb 4, 2020

Dosant changed the title ~~[wip] Move new_vis_modal to visualisations plugin~~ Move new_vis_modal to visualizations plugin Feb 4, 2020

Dosant marked this pull request as ready for review February 4, 2020 10:50

Dosant requested a review from a team February 4, 2020 10:50

Dosant requested review from a team as code owners February 4, 2020 10:50

clean up tests

9342bef

Dosant requested a review from ppisljar February 4, 2020 10:58

flash1293 approved these changes Feb 4, 2020

View reviewed changes

ppisljar reviewed Feb 4, 2020

View reviewed changes

review improvements

9dc0b46

1. expose showNewVisModal() on vis plugin 2. use it in visualise instead of custom directive

Dosant added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes labels Feb 4, 2020

ppisljar approved these changes Feb 4, 2020

View reviewed changes

fix typecheck

a8ec165

cchaos approved these changes Feb 5, 2020

View reviewed changes

Dosant added 2 commits February 5, 2020 16:14

improve style imports

56a615d

rename to _index.scss

bf5b890

cchaos suggested changes Feb 5, 2020

View reviewed changes

fix index imports

8bc6f8e

Dosant requested a review from cchaos February 5, 2020 17:54

cchaos approved these changes Feb 5, 2020

View reviewed changes

Merge branch 'master' into dev/vis-to-np/show-new-vis-modal

9adda44

Dosant merged commit 927be66 into elastic:master Feb 6, 2020

Dosant mentioned this pull request Feb 6, 2020

[7.x] Move new_vis_modal to visualizations plugin (#56654) #56956

Merged

Dosant mentioned this pull request Feb 6, 2020

Fix auto closing new Vis modal when selecting Lens or navigating away with browser history #56998

Merged

7 tasks

	@import './np_ready/public/wizard/_index';
	@import './np_ready/public/wizard/index';

	@import './np_ready/public';
	@import './np_ready/public/index';

Conversation

Dosant commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Dev Docs

Before

After

Checklist

For maintainers

Uh oh!

elasticmachine commented Feb 4, 2020

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

ppisljar Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

Dosant Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

Dosant commented Feb 5, 2020

Uh oh!

cchaos Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

Dosant Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

Dosant Feb 5, 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!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Dosant commented Feb 6, 2020

Uh oh!

kibanamachine commented Feb 6, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Dosant commented Feb 3, 2020 •

edited

Loading

Dosant Feb 5, 2020 •

edited

Loading