feat(explore): Show confirmation modal if user exits Explore without saving changes

[kgabryje]

SUMMARY

This PR adds a browser confirmation modal (similar to the one that we have in dashboard edit mode) when user makes a change in control panel and then tries to close Explore (either by closing the tab or moving to another page) without clicking Save button. The modal that displays when user navigates out of SPA context (some external page or a Superset page that hasn't been migrated to SPA yet) or closes the tab, is not customizable. In the case of navigating within SPA context, we can easily display a custom message in the modal thanks to react-router's Prompt component. However, displaying a custom modal instead of the default one rendered by <Prompt /> requires more effort and will be handled in a separate PR.

CC @kasiazjc

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

https://user-images.githubusercontent.com/15073128/188621562-23b4e1cf-32e7-440a-9b03-ae90cdf398d4.mov

TESTING INSTRUCTIONS

1. Open a chart and make some changes in control panel
2. Try to go to another SPA page (like charts list)
3. A modal should appear. Clicking Leave should discard your changes and go to next page. Clicking Cancel should close the modal.
4. Make some changes in control panel and try to close browser tab or go to some external page.
5. A default browser confirmation modal should appear (it's not customizable)
6. Save your changes
7. Now leaving the page should not trigger the confirmation modal

KNOWN LIMITATION: if the previous page is a SPA route (for example you opened Explore from charts list), clicking browser back button will not trigger the confirmation modal. It's very bugged in react-router and I decided to disable it for now. We'll try to find a workaround in the future

ADDITIONAL INFORMATION

• [ ] Has associated issue:
• [ ] Required feature flags:
• [ ] Changes UI
• [ ] Includes DB Migration (follow approval process in SIP-59)
  • [ ] Migration is atomic, supports rollback & is backwards-compatible
  • [ ] Confirm DB migration upgrade and downgrade tested
  • [ ] Runtime estimates and downtime expectations provided
• [ ] Introduces new feature or API
• [ ] Removes existing feature or API

[codecov[bot]]

Codecov Report

Merging #20902 (9d2d519) into master (29c233f) will decrease coverage by 0.01%. The diff coverage is 58.90%.

:exclamation: Current head 9d2d519 differs from pull request most recent head 1faa72c. Consider uploading reports for the commit 1faa72c to get more accurate results

@@            Coverage Diff             @@
##           master   #20902      +/-   ##
==========================================
- Coverage   66.56%   66.55%   -0.02%     
==========================================
  Files        1790     1791       +1     
  Lines       68393    68437      +44     
  Branches     7279     7284       +5     
==========================================
+ Hits        45526    45546      +20     
- Misses      20990    21012      +22     
- Partials     1877     1879       +2     

Flag	Coverage Δ
javascript	52.67% <58.90%> (-0.01%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...set-frontend/src/explore/actions/hydrateExplore.ts	42.10% <ø> (ø)
...rset-frontend/src/explore/components/SaveModal.tsx	39.04% <0.00%> (ø)
...-frontend/src/explore/reducers/saveModalReducer.js	27.27% <0.00%> (-2.73%)	:arrow_down:
...mponents/useRouteLeaveGuard/useRouteLeaveGuard.tsx	34.48% <34.48%> (ø)
...t-frontend/src/explore/actions/saveModalActions.js	96.87% <50.00%> (-1.52%)	:arrow_down:
.../explore/components/ExploreViewContainer/index.jsx	53.12% <64.28%> (+0.66%)	:arrow_up:
...rset-frontend/src/explore/exploreUtils/formData.ts	88.88% <91.66%> (+3.17%)	:arrow_up:
...-frontend/src/components/AlteredSliceTag/index.jsx	87.75% <100.00%> (-0.82%)	:arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jinghua-qa]

/testenv up

[github-actions[bot]]

@jinghua-qa Ephemeral environment spinning up at http://35.86.174.73:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[codyml]

@kgabryje I'm not sure if this is fixable or if it was like this before, but I noticed that if you have unsaved changes and try to navigate to Dashboard, even if you click "cancel" the navigation indicator stays on Dashboard:

[codyml]

@kgabryje Also not sure if this was intentional, but I noticed that if you edit the chart title but clicking directly on the rendered title and edit it, the "Altered" tag appears but it doesn't block navigation away.

[kgabryje]

@kgabryje I'm not sure if this is fixable or if it was like this before, but I noticed that if you have unsaved changes and try to navigate to Dashboard, even if you click "cancel" the navigation indicator stays on Dashboard

That's an existing bug - you can try it by going to charts list, then, dashboards list, then click "back" - dashboards will still be highlighted

[kgabryje]

@kgabryje Also not sure if this was intentional, but I noticed that if you edit the chart title but clicking directly on the rendered title and edit it, the "Altered" tag appears but it doesn't block navigation away.

Oops, well spotted!

[eschutho]

@jinghua-qa can we test what happens when the chart is running on a query? Does it show the correct save chart modal with the dialog to save a dataset?

[jinghua-qa]

/testenv up

[github-actions[bot]]

@jinghua-qa Ephemeral environment spinning up at http://18.236.190.110:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[jinghua-qa]

in the confirmation modal, is it make sense to have a cancel option or close dialog option? Looks like now only have discard and save, as user i may need to think about double check my change before i save, but i dont have an option currenlty.

[jinghua-qa]

I also noticed that there are different confirmation dialog, and this one did not have save option, only cancel and leave, is this expected? are we trying to have same confirmation dialog modal when user leave explore without save?

[kgabryje]

in the confirmation modal, is it make sense to have a cancel option or close dialog option? Looks like now only have discard and save, as user i may need to think about double check my change before i save, but i dont have an option currenlty.

I think it makes sense! @kasiazjc wdyt?

[kgabryje]

I also noticed that there are different confirmation dialog, and this one did not have save option, only cancel and leave, is this expected? are we trying to have same confirmation dialog modal when user leave explore without save?

Yes - our custom confirmation modal is possible to display only when navgating between SPA routes. When you try to close the tab or go to an external page, a default browser modal appears - it's not customizable in any way (it's a browser security measure)

[kasiazjc]

in the confirmation modal, is it make sense to have a cancel option or close dialog option? Looks like now only have discard and save, as user i may need to think about double check my change before i save, but i dont have an option currenlty.

I think it makes sense! @kasiazjc wdyt?

Good point, I'm thinking - maybe for consistency we should follow chrome flow actually instead of adding this one. Copy:

Your chart is not saved If you leave and don't save, changes will be lost. [Cancel][Leave]

Thoughts?

[jinghua-qa]

I think it makes sense! @kasiazjc wdyt?

Good point, I'm thinking - maybe for consistency we should follow chrome flow actually instead of adding this one. Copy:

Your chart is not saved If you leave and don't save, changes will be lost. [Cancel][Leave]

Thoughts?

Do we have a final plan for this design? My personal feeling is will be great to have all, [Cancel][Leave][Save], if that is too complicated, i think i will vote for [Cancel][Leave] which is consistent with the chrome flow, and i can always cancel and do save on explore.

[kasiazjc]

I think it makes sense! @kasiazjc wdyt?

Good point, I'm thinking - maybe for consistency we should follow chrome flow actually instead of adding this one. Copy: Your chart is not saved If you leave and don't save, changes will be lost. [Cancel][Leave] Thoughts?

Do we have a final plan for this design? My personal feeling is will be great to have all, [Cancel][Leave][Save], if that is too complicated, i think i will vote for [Cancel][Leave] which is consistent with the chrome flow, and i can always cancel and do save on explore.

I try to avoid adding 3 buttons as much as possible, as it can get quite confusing. After clicking "save" we would still have to open "save" modal, so I would say let's go with [Cancel][Leave] @kgabryje

[codyml]

I think it makes sense! @kasiazjc wdyt?

Good point, I'm thinking - maybe for consistency we should follow chrome flow actually instead of adding this one. Copy: Your chart is not saved If you leave and don't save, changes will be lost. [Cancel][Leave] Thoughts?

Do we have a final plan for this design? My personal feeling is will be great to have all, [Cancel][Leave][Save], if that is too complicated, i think i will vote for [Cancel][Leave] which is consistent with the chrome flow, and i can always cancel and do save on explore.

I try to avoid adding 3 buttons as much as possible, as it can get quite confusing. After clicking "save" we would still have to open "save" modal, so I would say let's go with [Cancel][Leave] @kgabryje

[Cancel] [Leave] makes the most sense to me too! As a user I think I'd expect there to be an option to just cancel the close.

[kgabryje]

Updated with Cancel/Leave

[kasiazjc]

Updated with Cancel/Leave

lgtm, thankss

[jinghua-qa]

/testenv up

[github-actions[bot]]

@jinghua-qa Ephemeral environment spinning up at http://34.211.247.176:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.