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

#144 confirm if unsaved form changes - #169

Open
tjfoerster wants to merge 3 commits into
masterfrom
unsaved-warning
Open

#144 confirm if unsaved form changes#169
tjfoerster wants to merge 3 commits into
masterfrom
unsaved-warning

Conversation

@tjfoerster

Copy link
Copy Markdown
Collaborator

Issue #144 done

@tjfoerster
tjfoerster requested a review from dboehmer May 18, 2022 00:19
Comment thread root/static/js/unsavedChanges.js Outdated
(e || window.event).returnValue = '';
return '';
}
}) No newline at end of file

Copy link
Copy Markdown
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

Comment thread root/static/js/unsavedChanges.js Outdated
Comment thread root/static/js/unsavedChanges.js Outdated
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
Copy Markdown
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?

Comment thread root/static/js/unsavedChanges.js Outdated
@@ -0,0 +1,21 @@
const forms = document.getElementsByClassName('confirmIfUnsavedChanges');

Copy link
Copy Markdown
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";?

Comment thread root/static/js/unsavedChanges.js Outdated

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

Copy link
Copy Markdown
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.

Comment thread lib/Coocook/Controller/Recipe.pm Outdated
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
Copy Markdown
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? 🤔

[% END %]

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

Copy link
Copy Markdown
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.

2 participants