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

[Visualize Editor] Add cancel button when navigating from Dashboard #77608

Merged
merged 14 commits into from
Oct 6, 2020

Conversation

majagrubic
Copy link
Contributor

@majagrubic majagrubic commented Sep 16, 2020

Summary

Fixes: #70650

When editing visualizations from Dashboard, currently there is no simple way to cancel editing and return to Dashboard. This PR addresses that, by adding a cancel button. This button is present both the old and new (by-value) flow.
cancel_button

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@majagrubic majagrubic changed the title Add cancel button in the visualize editor [Visualize Editor] Add cancel button when navigating from Dashboard Sep 18, 2020
@majagrubic majagrubic added release_note:roadmap v7.10.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Sep 18, 2020
@majagrubic majagrubic marked this pull request as ready for review September 18, 2020 09:38
@majagrubic majagrubic requested a review from a team September 18, 2020 09:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@majagrubic
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Tested locally in chrome, and mostly works as expected. One thing I noticed, though is that the 'unsaved changes' warning is triggered any time you press Cancel, even if you haven't changed anything, or if you've just created a blank visualization with no customizations.

I think this should be addressed, either by fixing it in this PR (see my code comments), or by letting this PR simply be the addition of a cancel button, with no confirmation, and creating a separate leave confirmation PR.

@majagrubic
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Thanks again for switching this out to use appLeaveHandler.

Test additions are looking great, and the confirm dialogue comes up mostly when expected with two small exceptions:

  • appearing on app leave after a 'create new' without any active changes.
  • appearing on save in by value mode.

Overall, LGTM once the CI is passing - tested locally on Chrome

@majagrubic
Copy link
Contributor Author

majagrubic commented Oct 6, 2020

The hook is based on hasUnsavedChanges flag, which will be set to true as soon as there are unsaved changes in the editor. And that's true for the initial creation; there IS an active change.

As for the second part, it's gonna be difficult to distinguish between "there is an unsaved change, but I want to discard the modal because there is a save in progress" vs "there is an unsaved change and I wish to show the modal".

@ThomThomson
Copy link
Contributor

The first problem may just come down to a difference in the 'unsaved changes' calculation between lens and visualize - which might be out of scope for this PR.

The second problem we also faced on lens, and found that the most explicit way to do this was to set the OnAppLeave handler to do nothing in the save and return method. Directly before the return redirect happens, you can call:

onAppLeave((actions) => {
  return actions.default();
});

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
visualize 271.4KB 273.7KB +2.3KB

page load bundle size

id before after diff
dashboard 570.2KB 570.2KB +11.0B

History

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

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Tested locally on chrome, everything works as expected!
LGTM.

@majagrubic majagrubic merged commit 0e89431 into elastic:master Oct 6, 2020
@majagrubic majagrubic deleted the cancel-button branch October 6, 2020 18:20
majagrubic pushed a commit that referenced this pull request Oct 6, 2020
…77608) (#79738)

* Add cancel button in the visualize editor

* Fixing i18n namespace

* Always show cancel button

* Always show cancel button

* Adding a fucntional test

* Show confirm dialog only if there are unsaved changes

* Show confirm modal only if there are changes

* Add onAppLeave handler and ditch confirmModal

* Fix functional test

* Only use onAppLeave if coming from dashboard/canvas

* Add actions.default to onSave and onSaveAndReturn

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard] Cancel button in editors to go back to the dashboard without saving the changes
4 participants