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

Dosant · 2020-02-06T16:07:30Z

Summary

Fix auto closing new Vis modal when selecting Lens or navigating away with browser history.
Regression appeared in #56654.

In src/legacy/core_plugins/kibana/public/visualize/np_ready/listing/visualize_listing.js I removed angular directive wrapper around NewVisModal component and started to use function exposed by visualisations plugin.

I didn't consider the case, that directive destroyed react component when angular scope was destroyed.

Checklist

Delete any items that are not applicable to this PR.

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
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

…ting away with browser history

elasticmachine · 2020-02-06T16:07:35Z

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

Dosant · 2020-02-06T16:09:30Z

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

@@ -145,8 +145,8 @@ class NewVisModal extends React.Component<TypeSelectionProps, TypeSelectionState
      params = this.props.addBasePath(visType.aliasUrl);
      if (this.props.editorParams && this.props.editorParams.includes('addToDashboard')) {
        params = `${params}?addToDashboard`;
-        this.props.onClose();


I am not sure, why this onClose was called specifically for aliasUrl with addToDashboard, but not just aliasUrl. This was the reason why for other vis types closing worked, but not for Lens

Could it be related to the modal having to stay open for OSS visualizations to pick the index pattern?

Edit: Probably not, forgot they are not an "aliasUrl". Maybe @majagrubic knows more

If I understand you correctly, then this seems like handled separately:

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

Line 123 in 3c429b1

private onVisTypeSelected = (visType: VisType | VisTypeAlias) => {

And then after search source is selected, code goes to that redirect function.

Dosant · 2020-02-06T16:10:08Z

src/legacy/core_plugins/kibana/public/visualize/np_ready/listing/visualize_listing.js

@@ -124,4 +124,10 @@ export function VisualizeListingController($injector, createNewVis) {
  this.listingLimit = uiSettings.get('savedObjects:listingLimit');

  addHelpMenuToAppChrome(chrome, docLinks);
+
+  $scope.$on('$destroy', () => {


This is added for cases when we navigate away without interacting with dialog. E.g. back button in browser which navigates us to other app.

flash1293

Tested and works for me 👍

Dosant · 2020-02-06T22:31:40Z

@elasticmachine merge upstream

elasticmachine · 2020-02-06T23:58:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 607d5f2

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

streamich

Code changes LGTM.

…ting away with browser history (elastic#56998) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…ting away with browser history (#56998) (#57092) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…b.com:jloleysens/kibana into console/feature/text-objects-in-saved-objects * 'console/feature/text-objects-in-saved-objects' of github.com:jloleysens/kibana: (103 commits) fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998) TS of esKuery\node_types (elastic#56857) Kibana app migration: Move static code dependencies into kibana_legacy plugin, part 1 (elastic#56408) Retry ES API calls that fail with 410/Gone (elastic#56950) [APM] Show missing permissions message to the user on the Services overview (elastic#56374) Fixing flaky CI tests for custom appRoutes (elastic#55763) [State Management][Docs] State syncing utils docs (elastic#56479) [Index management] Remove index mapper setting in tests (elastic#57066) Exposed common EuiExpressions to separate components be able to reuse for building new for Alert Types (elastic#56466) [SIEM] update url state between page if date is relative (elastic#56813) fix for chart_types test (elastic#57056) chore(NA): remove compress from dll minimizer (elastic#57023) [File upload] Migrate routing to NP & add route validation (elastic#52313) Adding docs for grouped nav advanced setting (elastic#57013) Use i18n titles for field formatters, human names for numeral locales (elastic#56348) [Maps] Remove EMS catalogue url from docs (elastic#57020) [Endpoint] ERT-82 ERT-83 ERT-84: Alert list API with pagination (elastic#56538) [DOCS] Adds Apple notarization info to install doc (elastic#57042) [ML] New Platform server shim: update results service routes to use new platform router (elastic#56886) Fix typo on detection engine rule (elastic#56993) ...

…t-state * upstream/master: (96 commits) top nav ts arg support (elastic#56984) [SIEM][detection engine] Limit network rules to filebeat source semantics (elastic#57130) Add docs for alerting and action settings (elastic#57035) Add Test to Verify Endpoint App Landing Page (elastic#57129) Update `markdown-to-jsx` (`6.9.3` → `6.11.0`) and `url-parse` (`1.4.4` → `1.4.7`) dependencies. (elastic#57126) chore(NA): removes use of parallel option in the terser minimizer (elastic#57077) [ML] New Platform server shim: update file data visualizer routes to use new platform router (elastic#56972) Specifying valid licenses for the Graph feature (elastic#55911) [APM][docs] Add troubleshooting for non-indexed fields (elastic#54948) [ML] DF Analytics creation: update schema definition for create route (elastic#56979) Remove Kibana a11y guide in favor of EUI (elastic#57021) [Logs UI] Set streamLive false in URL state when arriving from link-to (elastic#56329) [docs] Fix spaces api example json (elastic#50411) Add new config for filebeat index name (elastic#56920) [Metrics-UI] Fix toolbar popover for metrics table row (elastic#56796) Saved Objects testing (elastic#56965) Disabled categorization stats validation (elastic#57087) [Rollups] Server NP migration (elastic#55606) [Metrics UI] Limit group by selector to only 2 fields (elastic#56800) fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998) ...

* master: (96 commits) top nav ts arg support (elastic#56984) [SIEM][detection engine] Limit network rules to filebeat source semantics (elastic#57130) Add docs for alerting and action settings (elastic#57035) Add Test to Verify Endpoint App Landing Page (elastic#57129) Update `markdown-to-jsx` (`6.9.3` → `6.11.0`) and `url-parse` (`1.4.4` → `1.4.7`) dependencies. (elastic#57126) chore(NA): removes use of parallel option in the terser minimizer (elastic#57077) [ML] New Platform server shim: update file data visualizer routes to use new platform router (elastic#56972) Specifying valid licenses for the Graph feature (elastic#55911) [APM][docs] Add troubleshooting for non-indexed fields (elastic#54948) [ML] DF Analytics creation: update schema definition for create route (elastic#56979) Remove Kibana a11y guide in favor of EUI (elastic#57021) [Logs UI] Set streamLive false in URL state when arriving from link-to (elastic#56329) [docs] Fix spaces api example json (elastic#50411) Add new config for filebeat index name (elastic#56920) [Metrics-UI] Fix toolbar popover for metrics table row (elastic#56796) Saved Objects testing (elastic#56965) Disabled categorization stats validation (elastic#57087) [Rollups] Server NP migration (elastic#55606) [Metrics UI] Limit group by selector to only 2 fields (elastic#56800) fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998) ...

fix auto closing new vis modal when navigating to lens or when naviga…

3c429b1

…ting away with browser history

Dosant added bug Fixes for quality problems that affect the customer experience v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Feb 6, 2020

Dosant requested a review from a team February 6, 2020 16:07

Dosant requested a review from a team as a code owner February 6, 2020 16:07

Dosant requested review from majagrubic and flash1293 February 6, 2020 16:07

Dosant commented Feb 6, 2020

View reviewed changes

flash1293 approved these changes Feb 6, 2020

View reviewed changes

Merge branch 'master' into dev/auto-close-modal-lens

607d5f2

streamich approved these changes Feb 7, 2020

View reviewed changes

Dosant merged commit a7be72c into elastic:master Feb 7, 2020

Dosant added a commit to Dosant/kibana that referenced this pull request Feb 7, 2020

fix auto closing new vis modal when navigating to lens or when naviga…

68802f1

…ting away with browser history (elastic#56998) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Dosant mentioned this pull request Feb 7, 2020

[7.x] fix auto closing new vis modal when navigating to lens or when navigating away with browser history (#56998) #57092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Dosant commented Feb 6, 2020

elasticmachine commented Feb 6, 2020

Dosant Feb 6, 2020

flash1293 Feb 6, 2020 •

edited

Loading

Dosant Feb 6, 2020

Dosant Feb 6, 2020

flash1293 left a comment

Dosant commented Feb 6, 2020

elasticmachine commented Feb 6, 2020

streamich left a comment

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

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

Conversation

Dosant commented Feb 6, 2020

Summary

Checklist

For maintainers

elasticmachine commented Feb 6, 2020

Dosant Feb 6, 2020

Choose a reason for hiding this comment

flash1293 Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Dosant Feb 6, 2020

Choose a reason for hiding this comment

Dosant Feb 6, 2020

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

Dosant commented Feb 6, 2020

elasticmachine commented Feb 6, 2020

💚 Build Succeeded

streamich left a comment

Choose a reason for hiding this comment

flash1293 Feb 6, 2020 •

edited

Loading