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

kgabryje · 2022-07-28T13:57:39Z

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

Screen.Recording.2022-09-06.at.13.10.28.mov

TESTING INSTRUCTIONS

Open a chart and make some changes in control panel
Try to go to another SPA page (like charts list)
A modal should appear. Clicking Leave should discard your changes and go to next page. Clicking Cancel should close the modal.
Make some changes in control panel and try to close browser tab or go to some external page.
A default browser confirmation modal should appear (it's not customizable)
Save your changes
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 · 2022-07-28T16:35:32Z

Codecov Report

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

❗ 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%)`	⬇️

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%)`	⬇️
...mponents/useRouteLeaveGuard/useRouteLeaveGuard.tsx	`34.48% <34.48%> (ø)`
...t-frontend/src/explore/actions/saveModalActions.js	`96.87% <50.00%> (-1.52%)`	⬇️
.../explore/components/ExploreViewContainer/index.jsx	`53.12% <64.28%> (+0.66%)`	⬆️
...rset-frontend/src/explore/exploreUtils/formData.ts	`88.88% <91.66%> (+3.17%)`	⬆️
...-frontend/src/components/AlteredSliceTag/index.jsx	`87.75% <100.00%> (-0.82%)`	⬇️

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

jinghua-qa · 2022-07-29T08:10:33Z

/testenv up

github-actions · 2022-07-29T09:00:24Z

@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 · 2022-07-29T14:45:07Z

superset-frontend/src/components/RouteLeaveGuard/RouteLeaveGuard.tsx

+// eslint-disable-next-line import/no-extraneous-dependencies
+import { PromptProps } from 'react-router';
+
+const handleUnloadEvent = (e: BeforeUnloadEvent) => {


If someone tries to reuse this component to guard against route changes at two places in the same tree, I think there might be unexpected behavior because all addEventListener/removeEventListener calls from different instances of the component will reference the same function. How about moving it inside the component body so it's re-defined for each instantiation?

Good point!

codyml · 2022-07-29T14:59:25Z

@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 · 2022-07-29T15:01:44Z

@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 · 2022-07-29T16:26:25Z

@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 · 2022-07-29T16:26:40Z

@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 · 2022-07-29T18:01:52Z

@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 · 2022-08-01T07:02:40Z

/testenv up

github-actions · 2022-08-01T07:06:48Z

@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 · 2022-08-01T07:34:42Z

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 · 2022-08-01T07:38:31Z

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 · 2022-08-01T07:43:59Z

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 · 2022-08-01T07:45:45Z

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 · 2022-08-01T14:19:42Z

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 · 2022-08-03T21:40:26Z

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 · 2022-08-04T09:58:26Z

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 · 2022-08-04T15:13:12Z

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.

…saving changes

kgabryje · 2022-09-06T10:29:37Z

Updated with Cancel/Leave

kasiazjc · 2022-09-06T10:44:56Z

Updated with Cancel/Leave

lgtm, thankss

codyml · 2022-09-07T07:08:33Z

superset-frontend/src/explore/components/ExploreViewContainer/index.jsx

@@ -337,7 +375,7 @@ function ExploreViewContainer(props) {
  }

  function toggleModal() {
-    setShowingModal(!showingModal);
+    props.actions.setSaveModalVisible(!props.isSaveModalVisible);


I think this PR might be re-triggering the annoying bug that forced us to revert these two PRs:

Screen.Recording.2022-09-07.at.1.01.05.AM.mov

I don't think we were able to solve it: IIRC it seemed to happen whenever we tried to send additional Redux actions at the same time as when Explore was rendering. Here's the PR that would have fixed it if we hadn't reverted the whole thing. Not sure if your case is similar? I think that also might be why Cypress is failing on yours.

I also noticed that if I Save As and it doesn't refresh, it'll then show the modal every time even if I save it again.

jinghua-qa · 2022-09-16T22:50:52Z

/testenv up

github-actions · 2022-09-16T22:55:12Z

@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.

pull-request-size bot added the size/L label Jul 28, 2022

kgabryje requested review from michael-s-molina, geido, stephenLYZ and zhaoyongjie July 28, 2022 15:48

kgabryje added the need:qa-review Requires QA review label Jul 28, 2022

codyml reviewed Jul 29, 2022

View reviewed changes

kgabryje added 5 commits September 6, 2022 12:08

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

5d59217

…saving changes

Fix calling cleanup func unnecessarily

240495c

Fix comparing AdhocMetric instance with JSON object

a237436

Replace sliceFormData with the initial form data

ecca373

Block SPA routes

45c6ee2

kgabryje added 4 commits September 6, 2022 12:08

Change message, remove console logs

214637c

Fix lint

7faea80

Implement new design

c8fcb0b

Use Cancel/Leave

97b60e9

kgabryje force-pushed the feat/explore-exit-confirm branch from 6cd3a91 to 97b60e9 Compare September 6, 2022 10:29

Fix delay

1faa72c

codyml reviewed Sep 7, 2022

View reviewed changes

michael-s-molina closed this Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kgabryje commented Jul 28, 2022 •

edited

Loading

codecov bot commented Jul 28, 2022 •

edited

Loading

jinghua-qa commented Jul 29, 2022

github-actions bot commented Jul 29, 2022

codyml Jul 29, 2022

kgabryje Jul 29, 2022

codyml commented Jul 29, 2022

codyml commented Jul 29, 2022 •

edited

Loading

kgabryje commented Jul 29, 2022

kgabryje commented Jul 29, 2022

eschutho commented Jul 29, 2022

jinghua-qa commented Aug 1, 2022

github-actions bot commented Aug 1, 2022

jinghua-qa commented Aug 1, 2022

jinghua-qa commented Aug 1, 2022

kgabryje commented Aug 1, 2022

kgabryje commented Aug 1, 2022

kasiazjc commented Aug 1, 2022

jinghua-qa commented Aug 3, 2022

kasiazjc commented Aug 4, 2022

codyml commented Aug 4, 2022

kgabryje commented Sep 6, 2022

kasiazjc commented Sep 6, 2022

codyml Sep 7, 2022

codyml Sep 7, 2022

codyml Sep 7, 2022

jinghua-qa commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

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

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

Conversation

kgabryje commented Jul 28, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 28, 2022 • edited Loading

Codecov Report

jinghua-qa commented Jul 29, 2022

github-actions bot commented Jul 29, 2022

codyml Jul 29, 2022

Choose a reason for hiding this comment

kgabryje Jul 29, 2022

Choose a reason for hiding this comment

codyml commented Jul 29, 2022

codyml commented Jul 29, 2022 • edited Loading

kgabryje commented Jul 29, 2022

kgabryje commented Jul 29, 2022

eschutho commented Jul 29, 2022

jinghua-qa commented Aug 1, 2022

github-actions bot commented Aug 1, 2022

jinghua-qa commented Aug 1, 2022

jinghua-qa commented Aug 1, 2022

kgabryje commented Aug 1, 2022

kgabryje commented Aug 1, 2022

kasiazjc commented Aug 1, 2022

jinghua-qa commented Aug 3, 2022

kasiazjc commented Aug 4, 2022

codyml commented Aug 4, 2022

kgabryje commented Sep 6, 2022

kasiazjc commented Sep 6, 2022

codyml Sep 7, 2022

Choose a reason for hiding this comment

codyml Sep 7, 2022

Choose a reason for hiding this comment

codyml Sep 7, 2022

Choose a reason for hiding this comment

jinghua-qa commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

kgabryje commented Jul 28, 2022 •

edited

Loading

codecov bot commented Jul 28, 2022 •

edited

Loading

codyml commented Jul 29, 2022 •

edited

Loading