Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

#144 confirm if unsaved form changes #169

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

tjfoerster
Copy link
Collaborator

Issue #144 done

@tjfoerster tjfoerster requested a review from dboehmer May 18, 2022 00:19
(e || window.event).returnValue = '';
return '';
}
})
Copy link
Owner

Choose a reason for hiding this comment

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

Here’s a trailing newline missing

root/static/js/unsavedChanges.js Outdated Show resolved Hide resolved
Comment on lines 5 to 7
const inputs = Array.prototype.slice.apply(form.getElementsByTagName('input'));
const textareas = Array.prototype.slice.apply(form.getElementsByTagName('textarea'));
const selects = Array.prototype.slice.apply(form.getElementsByTagName('select'));
Copy link
Owner

Choose a reason for hiding this comment

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

Can you align those vertically?

@@ -0,0 +1,21 @@
const forms = document.getElementsByClassName('confirmIfUnsavedChanges');
Copy link
Owner

Choose a reason for hiding this comment

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

How about "use strict";?


window.addEventListener('beforeunload', (e) => {
if (hasUnsavedChanges) {
(e || window.event).returnValue = '';
Copy link
Owner

Choose a reason for hiding this comment

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

If you have a relevant web link from your research, please included it as a comment. I remember the pain of research and even with the correct answer couldn’t find the spec right now.

@@ -107,6 +107,7 @@ sub edit : GET HEAD Chained('base') PathPart('') Args(0) RequiresCapability('vie
and $c->stash(
import_url => $c->uri_for_action( '/browse/recipe/import', [ $recipe->id, $recipe->url_name ] ) );

push @{ $c->stash->{js} }, '/js/unsavedChanges.js';
Copy link
Owner

Choose a reason for hiding this comment

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

I like your generic implementation with the <form> class. Do you think we should simply add this to our global script.js to utilize it on any page? 🤔

@@ -13,7 +13,7 @@
</div>
[% END %]

<form method="post" action="[% update_url %]">
<form class="confirmIfUnsavedChanges" method="post" action="[% update_url %]">
Copy link
Owner

Choose a reason for hiding this comment

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

I checked out your branch and tested. The JS file was loaded but it had no effect. I made changes and could leave the page without warning.

@tjfoerster tjfoerster requested a review from dboehmer May 31, 2022 13:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants