-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fix confirmation modal on subdomain settings #12845
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
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.
PR Summary
Fixed confirmation modal behavior in workspace subdomain settings to properly handle conflict scenarios and form submission structure.
- Moved
ConfirmationModal
component outside the form element inpackages/twenty-front/src/pages/settings/workspace/SettingsDomain.tsx
to prevent form submission issues - Added
closeModal
fromuseModal
hook to properly dismiss the modal when subdomain conflicts occur - Improved error handling flow by ensuring modal closes when domain validation fails
1 file reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
<ConfirmationModal | ||
modalId={SUBDOMAIN_CHANGE_CONFIRMATION_MODAL_ID} | ||
title={t`Change subdomain?`} | ||
subtitle={t`You're about to change your workspace subdomain. This action will log out all users.`} | ||
onConfirmClick={() => { | ||
const values = form.getValues(); | ||
currentWorkspace && | ||
updateSubdomain(values.subdomain, currentWorkspace); | ||
}} | ||
/> |
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.
style: Consider using early return in onConfirmClick to avoid using && operator for conditional execution. Better pattern would be:
<ConfirmationModal | |
modalId={SUBDOMAIN_CHANGE_CONFIRMATION_MODAL_ID} | |
title={t`Change subdomain?`} | |
subtitle={t`You're about to change your workspace subdomain. This action will log out all users.`} | |
onConfirmClick={() => { | |
const values = form.getValues(); | |
currentWorkspace && | |
updateSubdomain(values.subdomain, currentWorkspace); | |
}} | |
/> | |
onConfirmClick={() => { | |
const values = form.getValues(); | |
if (!currentWorkspace) return; | |
updateSubdomain(values.subdomain, currentWorkspace); | |
}} |
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.
LGTM 👌
Thanks @ehconitin for your contribution! |
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:11015 This environment will automatically shut down when the PR is closed or after 5 hours. |
before -
2025-06-24.20-37-10.mov
after -
2025-06-24.20-54-32.mov