-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: modal form for reporting inaccurate Planning Constraints #3436
Conversation
Removed vultr server and associated DNS entries |
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.
A few minor comments and suggestions 🙂
editor.planx.uk/src/@planx/components/PlanningConstraints/Modal.tsx
Outdated
Show resolved
Hide resolved
editor.planx.uk/src/@planx/components/PlanningConstraints/Modal.tsx
Outdated
Show resolved
Hide resolved
setTextInput(e.target.value); | ||
}; | ||
|
||
const validateAndSubmit = () => { |
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.
One edge-case workflow -
- I mark a constraint as not applying
- I re-open modal to uncheck
- I can't save without error
- I have to cancel to get back to no overrides
This ties into the comment above on cancel behaviour - I think it is valid to submit this form with nothing checked, if the form is dirty.
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.
Can recreate and think I've got a solution for this one:
- if you open an empty form and click "Submit", I'll still trigger the validation error messages and prevent submission
- if you open a pre-populated form, clear your selected options and text input, and click "Submit" - I'll sync your updates to parent state (essentially "clear") and close the modal
- is it obvious you have to manually clear both your selected options and text input? Would you expect text input to auto-clear as soon as zero checklist options are selected? (getting into conditional question territory here)
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.
Working as expected!
One (very) minor issue is that errors persist from the modal being closed and opened - I guess they should be cleared on both submit and cancel?
Changes:
"OVERRIDE_CONSTRAINTS"
so we can work on followup PRs in parallelFollowup PRs:
onSubmit
& automationsTesting thread here: https://opensystemslab.slack.com/archives/C04DZ1NBUMR/p1721219483725279