-
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: exclusive 'Or' option in checklists #4056
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.
Extracted these test helpers out with their pointerEventsCheck
explanation to make the test files a little bit cleaner 🧼
3c313e5
to
56e3e75
Compare
editor.planx.uk/src/@planx/components/Checklist/Public/tests/mockOptions.ts
Outdated
Show resolved
Hide resolved
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.
Looking great!
I'm getting a ShareDB issue on the Pizza each time I make a change to the flow but can't recreate this locally. Each time I send a websocket request to ShareDB (e.g. updating a node) it times out and I end up with an endless spinner. This happens from time to time on pizzas or locally - destroying and rebuilding the Pizza should solve this.
Some minor comments on the code, no show stoppers.
A few things to consider -
The interaction with the allRequired
prop
It's possible to trigger this error state but not resolve it by checking the exclusive "or" option. We could resolve this via the UI but truth is that having allRequired
and an exclusive option or actually contradictory and we should expose this to Editors. I'd be in favour of adding this as a rule to the validation schema for this component.
Single exclusive option
This is currently enforced by the UI using the maxItems
prop. Again, I think this is worth being explicit about and having a check in our validation schema for this component to enforce. This is a bit more defensive and allows for UI drift in future without losing the intention of how this should work and behave.
editor.planx.uk/src/@planx/components/Checklist/Public/tests/Public.test.tsx
Show resolved
Hide resolved
@@ -28,3 +28,18 @@ export function getInitialExpandedGroups( | |||
[] as number[], | |||
); | |||
} | |||
|
|||
export const toggleCheckbox = ( |
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's possibly a number of places where this could be reused -
- Checkboxes in the List component
- Checkboxes in the Planning Constraints component
- I'm sure there's one or two more!
This is needed because we're not using Formik to totally control the state of these checkboxes using the Field
component so have to do it manually https://formik.org/docs/examples/checkboxes
This would be a nice follow up PR to try and address 👌
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.
Completely agree, I will follow-up on this in the next PR, and also split up some of the component files a bit more.
editor.planx.uk/src/@planx/components/Checklist/Editor/Options.tsx
Outdated
Show resolved
Hide resolved
editor.planx.uk/src/@planx/components/Checklist/Editor/Options.tsx
Outdated
Show resolved
Hide resolved
editor.planx.uk/src/@planx/components/Checklist/Public/Public.tsx
Outdated
Show resolved
Hide resolved
editor.planx.uk/src/@planx/components/Checklist/Public/tests/mockOptions.ts
Outdated
Show resolved
Hide resolved
checkbox | ||
), | ||
)} | ||
{exclusiveOrOption && ( |
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.
It could be quite nice to split some of these sections up into their components so that the structure if overall more discernable at a glance. Right now it's quite nested and complex to follow.
The catch here is that we need formik
in all of them.
We can certainly prop drill this along, or wrap the children in FormikContext
and then access the values via the useFormikContext()
hook. There's a good example of this in the invite to pay and GovPay metadata sections of the code base 👍
This can for sure be another PR or a fix-forward issue and doesn't need to be directly addressed here.
editor.planx.uk/src/@planx/components/Checklist/Editor/Options.tsx
Outdated
Show resolved
Hide resolved
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.
Appreciate the follow up changes here - all looking good!
🎟️ Trello ticket: https://trello.com/c/dXUW9EC1/3146-add-or-option-to-checklists
In this PR:
EXCLUSIVE_OR
maxLength
of 1.exclusiveOr
option is styled with an 'or' before it in public componentexclusiveOr
option is selectedStill to do...
Screenshots