search

pentacent / keila

Open Source Newsletter Tool.
https://keila.io
GNU Affero General Public License v3.0
1.36k stars 67 forks source link

Remind user of unsaved information #98

Closed gbottari closed 2 years ago

gbottari commented 2 years ago

This is a proposition to fix #65. The solution here is only applied to keila_web/templates/contact/edit.html.heex (edit contacts page) but could easily be extended to cover other pages.

The implementation is 100% javascript with AlpineJs so it won't mess with business logic. It uses the beforeunload event to show a message if the user has changed the form and the page is about to unload. It can also override this behavior so the message won't trigger when the user clicks the button for form submission.

@wmnnd if you like the solution here, I'll apply it on the other pages as well.

wmnnd commented 2 years ago

Oh that’s great, thanks for building this feature! One thing though: The warning string is not being translated … could you take a look at how to make sure the string is localized with gettext?

gbottari commented 2 years ago

Hey, we finished off the other edit pages. If you need anything else, just tell us!

wmnnd commented 2 years ago

Very cool, thanks! :pray:

I’ve got two questions: 1) Do you think it would make sense to use slightly more descriptive names for the hook functions? E.g. markFormUnsaved and markFormUnsaved? 2) Could you resolve the conflicts in this branch from merging #97?

gbottari commented 2 years ago

No problem! I rebased with main fixing all conflicts. For (1) I ended up refactoring it to setUnsavedReminder(val) where val can be either true or false.

wmnnd commented 2 years ago

Thanks for taking the initiative on this, @gbottari! I think this is a great UX improvement :clap: I did a small refactor to avoid the big indentation changes in the templates. I also came across some views that weren’t working correctly anymore because of how the AlpineJS nesting was set up (nested components don’t inherit the parent’s properties, so inserting a new component in between can break existing data flows).

Another thing I noticed was that I didn’t actually see the warning message you configured – turns out that all current browsers ignore the custom message and always show a default message, so I’ve removed the custom message to reflect that as well :slightly_smiling_face:

gbottari commented 2 years ago

Thank you for the kind words, @wmnnd !

Regarding the AlpineJS nesting, we noticed that this is not a problem anymore on V3. It may be worth considering updating to V3 to avoid breaking data flow with other components like you mentioned.

Another thing I noticed was that I didn’t actually see the warning message you configured – turns out that all current browsers ignore the custom message and always show a default message, so I’ve removed the custom message to reflect that as well slightly_smiling_face

I see what you mean! Sorry for letting that one slip. :sweat_smile: