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

[Time to Visualize] Fix Dashboard OnAppLeave #86193

Merged

Conversation

ThomThomson
Copy link
Contributor

Summary

A small fix that adds shows a confirm modal on leaving the dashboard application with unsaved changes.

This warning will not show when the user is redirecting to an editor via the create new button or via the edit link on a dashboard panel. This is accomplished with a new property in the embeddableStateTransfer service, used to determine whether or not a transfer is in progress.

Note Ignore the blue header, it's just a browser script to help tell kibana instances apart.
Dec-16-2020 15-56-08

Checklist

Delete any items that are not applicable to this PR.

For maintainers

…etermine whether or not to show onAppLeave confirm
@ThomThomson ThomThomson added Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0 v7.11.0 Project:TimeToVisualize labels Dec 16, 2020
@ThomThomson ThomThomson requested review from a team as code owners December 16, 2020 21:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@ThomThomson ThomThomson force-pushed the fix/dashboardConfirmOnAppLeave branch 2 times, most recently from 725f147 to cc3aded Compare December 17, 2020 21:33
Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

App Services code changes LGTM.

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Works as expected for me.

Just want to confirm, it's not possible to get into a state where there is a isTransgerInProgress where it's not immediately redirecting you to another app?

@ThomThomson
Copy link
Contributor Author

@crob611, This is a good point. I think the only way that could happen is if the transfer was cancelled, but there isn't yet code in place to cancel the state transfer once it's started. If we add any confirm modals or anything for the redirect, we will have to be aware of this.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 196.0KB 196.3KB +212.0B

Distributable file count

id before after diff
default 47297 48057 +760

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
embeddable 228.8KB 229.1KB +288.0B

History

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

@ThomThomson ThomThomson merged commit 8ce9b47 into elastic:master Dec 18, 2020
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Dec 18, 2020
Added isTransferInProgress to embeddable_state_transfer for apps to determine whether or not to show onAppLeave confirm
ThomThomson added a commit that referenced this pull request Dec 18, 2020
Added isTransferInProgress to embeddable_state_transfer for apps to determine whether or not to show onAppLeave confirm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Project:TimeToVisualize release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants