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

Warn users before they lose new changes #30632

Merged
merged 137 commits into from
May 19, 2023
Merged

Conversation

oisincoveney
Copy link
Contributor

@oisincoveney oisincoveney commented May 9, 2023

Closes #29421

Description

Adds browser-native warning messages when users try to refresh, close, or navigate away from the page when they have unsaved changes. The messages are displayed when the user is :

  • Editing a dashboard
  • Creating or editing an action
  • Editing an existing SQL question
  • Editing a model, be it the query definition or metadata
  • Admin: Creating or editing a database connection (only in Admin → Databases)
  • Admin: changing permissions
  • Admin: changing Data Model

How to verify

Instructions for verifying each event are explained in each PR listed below:

Demo

Upload a demo video or before/after screenshots if sensible or remove the section

Checklist

  • Tests have been added/updated to cover changes in this PR

This change is Reviewable

@mazameli mazameli self-requested a review May 16, 2023 15:13
Copy link
Contributor

@mazameli mazameli left a comment

Choose a reason for hiding this comment

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

All the test cases work as intended from my testing.

I think a follow-on to be considered is, when in explicit editing modes like with models or dashboards, whether we should also warn when the user attempts to navigate away from the page via the Back button or any other nav link. That might be annoying and unwelcome in more implicit edit states, like the SQL editor, but it feels like an odd omission to me especially in dashboard editing mode. cc @cdeweyx

One other little footnote: I was reminded that the Admin Permissions area does have a designed blocking modal if you attempt to navigate away without saving or cancelling changes explicitly:
image

@oisincoveney
Copy link
Contributor Author

All the test cases work as intended from my testing.

I think a follow-on to be considered is, when in explicit editing modes like with models or dashboards, whether we should also warn when the user attempts to navigate away from the page via the Back button or any other nav link. That might be annoying and unwelcome in more implicit edit states, like the SQL editor, but it feels like an odd omission to me especially in dashboard editing mode. cc @cdeweyx

One other little footnote: I was reminded that the Admin Permissions area does have a designed blocking modal if you attempt to navigate away without saving or cancelling changes explicitly: image

Absolutely - I had a look at the code for that modal, and it looks like it was made to be reused in other places but we just haven't done that yet.

@mazameli
Copy link
Contributor

@oisincoveney Actually, wait one moment — I think I found a bug. :/

Copy link
Contributor

@mazameli mazameli left a comment

Choose a reason for hiding this comment

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

We shouldn't be blocking refresh or window close for unsaved ad hoc GUI or native questions:

https://www.loom.com/share/a0cef8aab5d547d28fca4028335bb990

@cdeweyx
Copy link

cdeweyx commented May 16, 2023

All the test cases work as intended from my testing.
I think a follow-on to be considered is, when in explicit editing modes like with models or dashboards, whether we should also warn when the user attempts to navigate away from the page via the Back button or any other nav link. That might be annoying and unwelcome in more implicit edit states, like the SQL editor, but it feels like an odd omission to me especially in dashboard editing mode. cc @cdeweyx
One other little footnote: I was reminded that the Admin Permissions area does have a designed blocking modal if you attempt to navigate away without saving or cancelling changes explicitly: image

Absolutely - I had a look at the code for that modal, and it looks like it was made to be reused in other places but we just haven't done that yet.

@oisincoveney Would this take long as a follow-up? I don't think it needs to be in this branch necessarily, but I agree it's worth doing while we're looking at these areas. I think it makes sense for (cc: @mazameli to gut-check)

  • Editing a dashboard
  • Creating or editing an action
  • Editing an existing SQL question
  • Editing a model, be it the query definition or metadata

@mazameli
Copy link
Contributor

Editing an existing SQL question

@cdeweyx, this is the only one I'm unsure about. I could see someone opening up an existing SQL question, opening the editor to just futz around with it, or even just using one of the question's widgets/params (which "dirties" the question), and then annoyingly getting blocked by the modal when trying to leave.

The commonality across the contexts that I'm confident about adding an are-you-sure modal is that they're all explicit Create or Edit modes; while the implicit question editing/forking state is not.

@oisincoveney
Copy link
Contributor Author

All the test cases work as intended from my testing.
I think a follow-on to be considered is, when in explicit editing modes like with models or dashboards, whether we should also warn when the user attempts to navigate away from the page via the Back button or any other nav link. That might be annoying and unwelcome in more implicit edit states, like the SQL editor, but it feels like an odd omission to me especially in dashboard editing mode. cc @cdeweyx
One other little footnote: I was reminded that the Admin Permissions area does have a designed blocking modal if you attempt to navigate away without saving or cancelling changes explicitly: image

Absolutely - I had a look at the code for that modal, and it looks like it was made to be reused in other places but we just haven't done that yet.

@oisincoveney Would this take long as a follow-up? I don't think it needs to be in this branch necessarily, but I agree it's worth doing while we're looking at these areas. I think it makes sense for (cc: @mazameli to gut-check)

  • Editing a dashboard
  • Creating or editing an action
  • Editing an existing SQL question
  • Editing a model, be it the query definition or metadata

I don't think it would be too crazy, maybe around 1 week +/- a few days. A lot of the time spent on this PR was familiarizing myself with the code base, testing, etc rather than the actual code itself.

@oisincoveney
Copy link
Contributor Author

We shouldn't be blocking refresh or window close for unsaved ad hoc GUI or native questions:

https://www.loom.com/share/a0cef8aab5d547d28fca4028335bb990

Appreciate the loom, I'll get this sorted now

@mazameli
Copy link
Contributor

@oisincoveney One odd thing: when I create a new action, type anything in the editor and then click the back button, the modal remains open, but the page underneath the modal changes.

@mazameli
Copy link
Contributor

@oisincoveney also, if I'm editing/creating an action, then click outside the modal (which closes it), I don't see the "are you sure?" dialog. Is that in scope for this PR @cdeweyx?

@mazameli
Copy link
Contributor

"Add warning message when leaving an edited, existing SQL question #30477" doesn't seem to be addressed any longer, but I suspect that's due to my comments about how we shouldn't be blocking on ad hoc questions. The behavior in the branch feels right to me now, but now 30477 is not address so I just wanted to surface that to @cdeweyx

@cdeweyx
Copy link

cdeweyx commented May 18, 2023

@oisincoveney also, if I'm editing/creating an action, then click outside the modal (which closes it), I don't see the "are you sure?" dialog. Is that in scope for this PR @cdeweyx?

Not in scope for this

@cdeweyx
Copy link

cdeweyx commented May 18, 2023

"Add warning message when leaving an edited, existing SQL question #30477" doesn't seem to be addressed any longer, but I suspect that's due to my comments about how we shouldn't be blocking on ad hoc questions. The behavior in the branch feels right to me now, but now 30477 is not address so I just wanted to surface that to @cdeweyx

I'm on board with the behavior in the branch currently

@cdeweyx
Copy link

cdeweyx commented May 18, 2023

All the test cases work as intended from my testing.
I think a follow-on to be considered is, when in explicit editing modes like with models or dashboards, whether we should also warn when the user attempts to navigate away from the page via the Back button or any other nav link. That might be annoying and unwelcome in more implicit edit states, like the SQL editor, but it feels like an odd omission to me especially in dashboard editing mode. cc @cdeweyx
One other little footnote: I was reminded that the Admin Permissions area does have a designed blocking modal if you attempt to navigate away without saving or cancelling changes explicitly: image

Absolutely - I had a look at the code for that modal, and it looks like it was made to be reused in other places but we just haven't done that yet.

@oisincoveney Would this take long as a follow-up? I don't think it needs to be in this branch necessarily, but I agree it's worth doing while we're looking at these areas. I think it makes sense for (cc: @mazameli to gut-check)

  • Editing a dashboard
  • Creating or editing an action
  • Editing an existing SQL question
  • Editing a model, be it the query definition or metadata

I don't think it would be too crazy, maybe around 1 week +/- a few days. A lot of the time spent on this PR was familiarizing myself with the code base, testing, etc rather than the actual code itself.

@oisincoveney Let's leave modals out of scope. I'll create a follow-up issue to take that on sometime in the future. Doesn't need to be a part of this PR.

@oisincoveney oisincoveney merged commit bf49ab3 into master May 19, 2023
93 of 94 checks passed
@oisincoveney oisincoveney deleted the 29421-integration-branch branch May 19, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
visual Run Percy visual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Epic] Warn users before they lose new changes
5 participants