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

feature: Alert users when form has unsaved changes #3346

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

coolprobn
Copy link

@coolprobn coolprobn commented Oct 17, 2024

Description

Add a stimulus controller to track changes in the form and show alert box when user moves away from the current form, either by reloading the page or clicking any other buttons in the application with the use of Turbo Navigation.

Fixes # (issue)

Closes #673

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

https://www.loom.com/share/9bac6ed248014dfdbcc52d76b1973a99?sid=ca6389a5-eda4-41df-a357-e987a3565233

Manual review steps

Feature walkthrough is in the screen share attached above

TODO

  • Add tests for the changes

Copy link

codeclimate bot commented Oct 17, 2024

Analysis results are not available for those commits

View more on Code Climate.

Copy link
Collaborator

@adrianthedev adrianthedev left a comment

Choose a reason for hiding this comment

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

This PR is definitely in the right direction.

My wondering developer brain started to think about things.
I'm just going to add things here; feel free to ingore them.

  1. Would it be difficult to capture which fields are dirty?
  2. Would it be possible to "undirty" the form if the user changes the field back to the original value
  3. Would it be possible (or valuable) to show a dirty state (toggleable) through a setting.

All of these kind of converge into the same thing;
to hold previous and new state in JS and compare them.

It brings quite some complexity around it with state that can get dirty from different JS plugins.

But I can't shake that feeling that if I make the form back the way it was before, I'd like the dirty state to go away and not make me anxious of why I need to save the form, or loose my changes (whichever they are).

Do you get this feeling?
Do you think it's just me?

Thanks for the great work.

@RocKhalil
Copy link
Contributor

@adrianthedev jumping into the conversation cause I have other concerns regarding what you said;

what about custom fields developed by people for their own solutions ?
should the dirty state be accessible by other components to push to it ?
should relationships ( nested_attributes ) be taking into consideration ?
etc etc..

@adrianthedev
Copy link
Collaborator

Yes, I agree it gets deep very quickly.
Let's keep it simple.
I wonder if we could achieve that "un-dirty" use-case in a... simple way.

@coolprobn
Copy link
Author

@adrianthedev I had a fleeting thought on what if the field is reverted back to the original state but forgot about it soon. So, I get what you are talking about.

Tracking full state and toggling the form will be way out of scope but un-dirtying should be handled in this PR. I will work on that tomorrow and also start adding tests since this is the intended feature you were looking for.

Any tip on where to start to ensure I haven't broken existing tests? Running whole suite would take a lot of time, I assume?

@coolprobn
Copy link
Author

@adrianthedev Form un-dirtying is now implemented.

While testing though changes (both dirtying/undirtying) are not being tracked for Markdown fields, I will have to look into the implementation details for Markdowns which will take some time than I had initially anticipated so progress over the days could be slow here.

@coolprobn
Copy link
Author

@adrianthedev Implementation of the code portion is complete. Can you re-review when you have time?
I will start working on tests next time I work on this again.

I have tested this in possible fields I could find in the app:

  • Text field
  • Text area field
  • Number field
  • Checkbox Toggle
  • Dropdown
  • Code field
  • Markdown Editor - Easy MDE, Trix and Tiptap
  • Markdown editor with File upload -> for edit since file upload isn't possible in create form
  • Belongs to Field

@adrianthedev
Copy link
Collaborator

This is still on our plate to review @coolprobn. Sorry for the delay.

@coolprobn
Copy link
Author

This is still on our plate to review @coolprobn. Sorry for the delay.

No worries Adrian, I have caught yet another bug in the implementation which interrupts any turbo action and shows the alert box e.g. "modal" or those additional "update" actions that don't move away from the form page.

But I haven't been able to work on this after I asked for review as well due to work stuffs, hopefully I will get some time soon again to get back to this.

@coolprobn
Copy link
Author

@adrianthedev This PR is ready to review now from my side. Tests have also been added for the changes.

@coolprobn coolprobn changed the title WIP: feature: Alert users when form has unsaved changes feature: Alert users when form has unsaved changes Nov 15, 2024
@Paul-Bob Paul-Bob self-requested a review November 15, 2024 09:11
@adrianthedev
Copy link
Collaborator

Hey @coolprobn.
I seem to face some issues on the back button. Can you see them too?

https://www.loom.com/share/742a7099a390493dab6860f8977b81a3?sid=c94220f0-936e-4b49-9056-6b505559598d

@coolprobn
Copy link
Author

Hey @coolprobn. I seem to face some issues on the back button. Can you see them too?

https://www.loom.com/share/742a7099a390493dab6860f8977b81a3?sid=c94220f0-936e-4b49-9056-6b505559598d

Thanks for the review.

You are right, we aren't listening to events for page navigations like front/back button in the browser. Right now it works for page refresh or other in-page navigations with turbo. I will take a look at the back button when I work on this late this week or early next week.

Thanks for the patience.

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.

Alert users when they're navigating away from a form that has unsaved changes
3 participants