-
Notifications
You must be signed in to change notification settings - Fork 21
useBlocker prevent user from leaving dirty form
#1523
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
Conversation
| if (blocker.state === 'blocked' && !isDirty) { | ||
| blocker.reset() | ||
| } | ||
| }, [blocker, isDirty]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised this is necessary but I see it's from their example.
just-be-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is small enough and feels like a net improvement so I'm fine w/ starting here. I think we have to be really careful with populating fields on large forms because the user may populate something lower down in the form, leave, come back and create a new resource that does something unanticipated because they missed the population. I know that's an edge case but for the moment this feels better.
| // SideModalForm from inside another form, in which case submitting | ||
| // the inner form submits the outer form unless we stop propagation | ||
| e.stopPropagation() | ||
| form.reset({} as TFieldValues, { keepValues: true }) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. We should see how it feels and reevaluate in a week or two, maybe we want to use it everywhere. Might be good to note somewhere in the codebase what the limitations are — from my testing it doesn't block refreshes and it doesn't block closing the tab.
Visually I think this is a little flat and wide, like a McDonald's hamburger — could be narrower, maybe center the text?
|
That's the most american comparison this week. |
|
🍔 |
Fixes #996
Uses
useBlockerto prevent user from leaving dirty form. As discussed this isn't foolproof but I think good enough to catch most accidental exits from thefullPageForm.Related to #1522. The rationale for two approaches on handling unsubmitted forms and persistent form state discussed here: #1522 (comment)
That said, the difference between the two is a little fuzzy, we could also nix the idea of
useBlockercompletely and use persistent state for thefullPageFormalso. I'm open to either solution for this form. What do you think @david-crespo @zephraph ?