Skip to content

Conversation

@halfwit
Copy link
Member

@halfwit halfwit commented Sep 28, 2024

This creates a confirm dialog on clicking any navigation link while edits are still active.

  • This currently doesn't alert if a back/forward navigation occurs
Screenshot 2024-09-28 at 14 17 52

Copy link
Member

@itsrachelfish itsrachelfish 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 both Firefox and Chrome. These changes work insofar as the user is alerted when navigating away from the edit page however I found three problems:

  1. A warning message is displayed when saving the page that is being edited. This happens both when you click the save button as well as the preview button. There should not be a warning message when clicking save or preview. This is a major problem and needs to be addressed before this PR can be merged.

  2. When the event handler for clicking on a link is triggered and confirmed, the beforeunload logic still fires and displays another warning message. Why is it necessary to have two separate event handlers? Isn't simply using beforeunload enough?

  3. The custom confirmation message is not displayed in the beforeunload event handler. This is a minor issue but would be nice to fix while you are working on the other two.

Comment on lines +167 to +173
$(window).on('beforeunload', function(e) {
if (hasChanges(changed, formData)) {
var confirmationMessage = 'You have unsaved changes. Are you sure you want to leave?';
(e || window.event).returnValue = confirmationMessage;
return confirmationMessage;
}
});
Copy link
Member

Choose a reason for hiding this comment

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

This return message logic doesn't seem to be working? The beforeunload event is working, but I'm getting a generic warning message from my browser, not the custom message defined in the code.

Tested in both Firefox:
image

And Chrome:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants