Skip to content
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

Merged
merged 31 commits into from
Dec 18, 2024
Merged

feat: exclusive 'Or' option in checklists #4056

merged 31 commits into from
Dec 18, 2024

Conversation

jamdelion
Copy link
Contributor

@jamdelion jamdelion commented Dec 9, 2024

🎟️ Trello ticket: https://trello.com/c/dXUW9EC1/3146-add-or-option-to-checklists

In this PR:

  • Added a feature flag for this: EXCLUSIVE_OR
  • Added another listManager to Checklist editor modal to add an 'exclusive or' option when there is at least one other option. It has a maxLength of 1.
  • exclusiveOr option is styled with an 'or' before it in public component
  • Other checkboxes become deselected if the exclusiveOr option is selected

⚠️ Note: grouped options currently not accounted for in this PR - the exclusiveOr select does not appear when 'expandable' is toggled.

Still to do...

  • A11y review
  • Deal with the grouped options case

Screenshots

Before After
Options editor before an exclusive option has been added
Options editor with exclusive option added - button is greyed out
Public facing checklist

@jamdelion jamdelion changed the title Add exclusive-or select input and validation feat: exclusive 'Or' option in checklists Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

Removed vultr server and associated DNS entries

Copy link
Contributor Author

@jamdelion jamdelion Dec 10, 2024

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 🧼

@jamdelion jamdelion marked this pull request as ready for review December 10, 2024 15:46
Base automatically changed from jh/checklist-with-or-after-refactor to main December 12, 2024 10:49
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a 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.

image

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/shared/index.ts Outdated Show resolved Hide resolved
@@ -28,3 +28,18 @@ export function getInitialExpandedGroups(
[] as number[],
);
}

export const toggleCheckbox = (
Copy link
Contributor

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 👌

Copy link
Contributor Author

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.

checkbox
),
)}
{exclusiveOrOption && (
Copy link
Contributor

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.

@jamdelion jamdelion requested a review from DafyddLlyr December 18, 2024 12:38
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a 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!

@jamdelion jamdelion merged commit 2a7be85 into main Dec 18, 2024
12 checks passed
@jamdelion jamdelion deleted the jh/actual-or-work branch December 18, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants