-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(dashboard): Form blocker fix #7710
base: next
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
f1cac5c
to
2e51633
Compare
❌ Deploy Preview for dashboard-v2-novu-staging failed. Why did it fail? →
|
2e51633
to
be18e18
Compare
be18e18
to
321053b
Compare
321053b
to
06d70fe
Compare
06d70fe
to
2610fea
Compare
if (!ref.current) { | ||
const documentObserver = new MutationObserver(() => { | ||
if (ref.current) { | ||
documentObserver.disconnect(); | ||
setupElementObserver(ref.current); | ||
} | ||
}); | ||
|
||
documentObserver.observe(document.body, { | ||
childList: true, | ||
subtree: true, | ||
}); | ||
|
||
return () => { | ||
documentObserver.disconnect(); | ||
}; | ||
} |
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 part of the code could be removed if there is no element you didn't use the hook properly, and just do nothing
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.
Cleaned it up
const protectedOnOpenChange = (open: boolean) => { | ||
if (isDirty) { | ||
setShowAlert(true); | ||
} else { | ||
onOpenChange?.(open); | ||
} | ||
}; |
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 function is a bit tied to the drawer/modal use; what we might want to do instead is to validate if we are allowed to do some action, for example, close the drawer or navigate to a different page.
So if we change this to const { triggerIsAllowed, ProtectionAlert } = useFormProtection(...)
, then it will be more flexible (names for you to decide).
const onProceed = () => setDrawerOpened(false);
const { triggerIsAllowed, ProtectionAlert } = useFormProtection({
ref,
onProceed,
});
// similarly could be used with navigation or anything
const onDrawerOpenChange = (open: boolean) => {
if (triggerIsAllowed()) {
setDrawerOpen(open);
}
}
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.
There is also an onProceed
prop . You don't have to pass an onOpenChange
and can choose to use the protectedOnChange
only when you do.
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.
We don't need to have two props for different use cases.
Also the thing is that you always need to call protectedOnOpenChange
even if you use onProceed
and in case of navigation it's weird (naming).
}; | ||
|
||
const ProtectionAlert = useCallback( | ||
() => ( |
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.
btw this will create an anonymous react element in the tree
2610fea
to
0487af2
Compare
0487af2
to
952063f
Compare
What changed? Why was the change needed?
This PR fixes the blockers in our forms and also provides a unified way to protect forms.
<FormRoot />
which renders a<form />
and adds adata-dirty
attribute when dirty.useFormProtection
hooks receives a ref and detects when a dirty form exists as a descendant. Also protects from refreshes.<FormProtection />
is returned from the hook renders our custom alert automatically.Example usage: