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
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
a70d00f
Refactor Checklist folder into subfolders
jamdelion Dec 4, 2024
32689a2
More refactor
jamdelion Dec 4, 2024
bf201d3
Refactor editor
jamdelion Dec 4, 2024
2aabdc6
refactor optionsEditor into shared component
jamdelion Dec 4, 2024
cc9fe24
retrigger checks
jamdelion Dec 5, 2024
db6fe7f
Remove redundant file
jamdelion Dec 5, 2024
63eb8f2
Refactor to use BaseOptionseditor instead
jamdelion Dec 5, 2024
dacbb0f
consistent renaming
jamdelion Dec 5, 2024
688bff6
Tidy up
jamdelion Dec 5, 2024
0683fd5
Add exclusive-or select input and validation
jamdelion Dec 9, 2024
2498c3c
Add or styling before exclusiveOr option, plus test
jamdelion Dec 9, 2024
155dd91
Add another test
jamdelion Dec 9, 2024
76fa41c
Reorder options when exclusive or option selected
jamdelion Dec 9, 2024
3491906
Only show dropdown when more than one option
jamdelion Dec 10, 2024
2bbf8ee
Some tidying
jamdelion Dec 10, 2024
60f159d
Use a list manager for exclusive or instead of inputSelect
jamdelion Dec 10, 2024
f434f3f
Restrict exclusiveOr to one option only
jamdelion Dec 10, 2024
9e582f3
Fix tests
jamdelion Dec 10, 2024
6ccff9f
Remove reorderOptions function
jamdelion Dec 10, 2024
9ba4336
Grey out other options if exclusive option is checked
jamdelion Dec 11, 2024
919b408
Add exclusive flag to Option type so that exclusive option shows in n…
jamdelion Dec 12, 2024
56e3e75
Delete exclusive option if no normal options
jamdelion Dec 12, 2024
f892565
Merge branch 'main' into jh/actual-or-work
jamdelion Dec 12, 2024
40e7427
Feature flag it
jamdelion Dec 12, 2024
1203531
Resolve some code nits
jamdelion Dec 17, 2024
4f27259
Disable drag and drop and tidy code slightly
jamdelion Dec 17, 2024
0b61774
Refactor to use getExclusiveOptions function
jamdelion Dec 17, 2024
687ad69
use lodash partition
jamdelion Dec 17, 2024
3d3c9c7
Use partition on public side
jamdelion Dec 17, 2024
9585660
Refactor changeCheckbox function
jamdelion Dec 17, 2024
ef50330
add validation for allRequired and maxItems
jamdelion Dec 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { toggleExpandableChecklist } from "../model";
import { ChecklistProps } from "../types";
import { Options } from "./Options";

export const ChecklistComponent: React.FC<ChecklistProps> = (props) => {
export const ChecklistEditor: React.FC<ChecklistProps> = (props) => {
const type = TYPES.Checklist;

const formik = useFormik<Checklist>({
Expand Down Expand Up @@ -189,4 +189,4 @@ export const ChecklistComponent: React.FC<ChecklistProps> = (props) => {
);
};

export default ChecklistComponent;
export default ChecklistEditor;
63 changes: 61 additions & 2 deletions editor.planx.uk/src/@planx/components/Checklist/Editor/Options.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import Delete from "@mui/icons-material/Delete";
import Box from "@mui/material/Box";
import Button from "@mui/material/Button";
import IconButton from "@mui/material/IconButton";
import { BaseOptionsEditor } from "@planx/components/shared/BaseOptionsEditor";
import adjust from "ramda/src/adjust";
import compose from "ramda/src/compose";
import remove from "ramda/src/remove";
Expand All @@ -17,6 +18,10 @@ import type { Group } from "../model";
import ChecklistOptionsEditor from "./OptionsEditor";

export const Options: React.FC<{ formik: FormikHookReturn }> = ({ formik }) => {
const exclusiveOrOptionManagerShouldRender =
formik.values.options?.filter((opt: Option) => opt.data.exclusive !== true)
.length > 0;
jamdelion marked this conversation as resolved.
Show resolved Hide resolved

return (
<ModalSectionContent subtitle="Options">
{formik.values.groupedOptions ? (
Expand Down Expand Up @@ -128,9 +133,23 @@ export const Options: React.FC<{ formik: FormikHookReturn }> = ({ formik }) => {
</Box>
) : (
<ListManager
values={formik.values.options || []}
values={
formik.values.options?.filter(
(opt: Option) => !opt.data.exclusive,
) || []
}
onChange={(newOptions) => {
formik.setFieldValue("options", newOptions);
const exclusiveOptions =
formik.values.options?.filter(
(opt: Option) => opt.data.exclusive,
) || [];

const newCombinedOptions =
newOptions.length === 0
? []
: [...exclusiveOptions, ...newOptions];

formik.setFieldValue("options", newCombinedOptions);
}}
newValueLabel="add new option"
newValue={() =>
Expand All @@ -146,6 +165,46 @@ export const Options: React.FC<{ formik: FormikHookReturn }> = ({ formik }) => {
editorExtraProps={{ showValueField: !!formik.values.fn }}
/>
)}
{exclusiveOrOptionManagerShouldRender ? (
<Box mt={1}>
<ListManager
jamdelion marked this conversation as resolved.
Show resolved Hide resolved
values={
formik.values.options?.filter(
(opt: Option) => opt.data.exclusive,
) || []
}
onChange={(newExclusiveOptions) => {
const nonExclusiveOptions: Option[] =
formik.values.options?.filter(
(opt: Option) => !opt.data.exclusive,
) || [];
jamdelion marked this conversation as resolved.
Show resolved Hide resolved

const newCombinedOptions = [
...nonExclusiveOptions,
...newExclusiveOptions,
];

formik.setFieldValue("options", newCombinedOptions);
}}
newValueLabel='add "or" option'
maxItems={1}
newValue={() =>
({
data: {
text: "",
description: "",
val: "",
exclusive: true,
},
}) as Option
DafyddLlyr marked this conversation as resolved.
Show resolved Hide resolved
}
Editor={BaseOptionsEditor}
editorExtraProps={{ showValueField: !!formik.values.fn }}
/>
</Box>
) : (
<></>
)}
</ModalSectionContent>
);
};
122 changes: 84 additions & 38 deletions editor.planx.uk/src/@planx/components/Checklist/Public/Public.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Box from "@mui/material/Box";
import Grid from "@mui/material/Grid";
import Typography from "@mui/material/Typography";
import { visuallyHidden } from "@mui/utils";
import {
checklistValidationSchema,
Expand All @@ -21,7 +22,11 @@ import { object } from "yup";

import { Props } from "../types";
import { AutoAnsweredChecklist } from "./AutoAnsweredChecklist";
import { getInitialExpandedGroups, toggleInArray } from "./helpers";
import {
getInitialExpandedGroups,
toggleCheckbox,
toggleInArray,
} from "./helpers";

export enum ChecklistLayout {
Basic,
Expand Down Expand Up @@ -90,18 +95,36 @@ const VisibleChecklist: React.FC<Props> = (props) => {
const layout = getLayout({ options, groupedOptions });
const flatOptions = getFlatOptions({ options, groupedOptions });

const changeCheckbox = (id: string) => (_checked: any) => {
const newCheckedIds = formik.values.checked.includes(id)
? formik.values.checked.filter((x) => x !== id)
: [...formik.values.checked, id];

formik.setFieldValue(
"checked",
newCheckedIds.sort((a, b) => {
const originalIds = flatOptions.map((cb) => cb.id);
return originalIds.indexOf(a) - originalIds.indexOf(b);
}),
);
const sortCheckedIds = (ids: string[]): string[] => {
const originalIds = flatOptions.map((cb) => cb.id);
return ids.sort((a, b) => originalIds.indexOf(a) - originalIds.indexOf(b));
};

const exclusiveOrOption =
options?.find((option) => option.data?.exclusive === true) || undefined;
jamdelion marked this conversation as resolved.
Show resolved Hide resolved

const exclusiveOptionIsChecked =
exclusiveOrOption && formik.values.checked.includes(exclusiveOrOption.id);

const changeCheckbox = (id: string) => () => {
jamdelion marked this conversation as resolved.
Show resolved Hide resolved
const currentIds = formik.values.checked;
let newCheckedIds;
const currentCheckboxIsExclusiveOption =
exclusiveOrOption && id === exclusiveOrOption.id;

if (currentCheckboxIsExclusiveOption) {
newCheckedIds = exclusiveOptionIsChecked ? [] : [id];
} else if (exclusiveOrOption) {
newCheckedIds = toggleCheckbox(id, currentIds);
const onlyNonExclusiveOptions = newCheckedIds.filter(
(id: string) => exclusiveOrOption && id !== exclusiveOrOption.id,
);
newCheckedIds = onlyNonExclusiveOptions;
} else {
newCheckedIds = toggleCheckbox(id, currentIds);
}
const sortedCheckedIds = sortCheckedIds(newCheckedIds);
formik.setFieldValue("checked", sortedCheckedIds);
};

return (
Expand All @@ -122,36 +145,59 @@ const VisibleChecklist: React.FC<Props> = (props) => {
component="fieldset"
>
<legend style={visuallyHidden}>{text}</legend>
{options?.map((option) =>
layout === ChecklistLayout.Basic ? (
<FormWrapper key={option.id}>
<Grid item xs={12} key={option.data.text}>
<ChecklistItem
onChange={changeCheckbox(option.id)}
label={option.data.text}
{options
?.filter((option) => option.data.exclusive !== true)
jamdelion marked this conversation as resolved.
Show resolved Hide resolved
.map((option) =>
layout === ChecklistLayout.Basic ? (
<FormWrapper key={option.id}>
<Grid item xs={12} key={option.data.text}>
<ChecklistItem
onChange={changeCheckbox(option.id)}
label={option.data.text}
id={option.id}
checked={
formik.values.checked.includes(option.id) &&
!exclusiveOptionIsChecked
}
/>
</Grid>
</FormWrapper>
) : (
<Grid
item
xs={12}
sm={6}
contentWrap={4}
key={option.data.text}
>
<ImageButton
title={option.data.text}
id={option.id}
checked={formik.values.checked.includes(option.id)}
img={option.data.img}
selected={formik.values.checked.includes(option.id)}
onClick={changeCheckbox(option.id)}
checkbox
/>
</Grid>
</FormWrapper>
) : (
<Grid
item
xs={12}
sm={6}
contentWrap={4}
key={option.data.text}
>
<ImageButton
title={option.data.text}
id={option.id}
img={option.data.img}
selected={formik.values.checked.includes(option.id)}
onClick={changeCheckbox(option.id)}
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.

<FormWrapper key={exclusiveOrOption.id}>
<Grid item xs={12} key={exclusiveOrOption.data.text}>
<Typography width={36} display="flex" justifyContent="center">
or
</Typography>

<ChecklistItem
onChange={changeCheckbox(exclusiveOrOption.id)}
label={exclusiveOrOption.data.text}
id={exclusiveOrOption.id}
checked={formik.values.checked.includes(
exclusiveOrOption.id,
)}
/>
</Grid>
),
</FormWrapper>
)}

{groupedOptions && (
Expand Down
15 changes: 15 additions & 0 deletions editor.planx.uk/src/@planx/components/Checklist/Public/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.

thisCheckboxId: string,
currentlyCheckedOptionIds: string[],
) => {
const toggleOff = currentlyCheckedOptionIds.filter(
(id) => id !== thisCheckboxId,
);

const toggleOn = [...currentlyCheckedOptionIds, thisCheckboxId];

return currentlyCheckedOptionIds.includes(thisCheckboxId)
? toggleOff
: toggleOn;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import { screen } from "@testing-library/react";
import React from "react";
import { setup } from "testUtils";
import { vi } from "vitest";

import Checklist from "../Public";
import { optionsWithExclusiveOption } from "./mockOptions";
import { pressContinue, pressOption } from "./testUtils";

describe("when a user selects the exclusive 'or' option and nothing else", () => {
it("does not throw an error on submit", async () => {
const handleSubmit = vi.fn();

setup(
<Checklist
allRequired={false}
description=""
text="Which permanent structures have you lived in?"
handleSubmit={handleSubmit}
options={optionsWithExclusiveOption}
/>,
);
expect(screen.getByRole("heading")).toHaveTextContent(
"Which permanent structures have you lived in?",
);

await pressOption("Tent");

await pressContinue();

expect(handleSubmit).toHaveBeenCalledWith({
answers: ["tent_id"],
});
});
});

describe.each([
{
optionType: "a non-exclusive",
optionLabelText: "Caravan",
oppositeOptionLabelText: "Tent",
},
{
optionType: "an exclusive",
optionLabelText: "Tent",
oppositeOptionLabelText: "Caravan",
},
])(
"when a user selects $optionType option",
({ oppositeOptionLabelText, optionLabelText }) => {
it("deselects it when the opposite type of option is selected", async () => {
const { getByLabelText } = setup(
<Checklist
allRequired={false}
description=""
text="Which permanent structures have you lived in?"
options={optionsWithExclusiveOption}
/>,
);
const firstSelectedOption = getByLabelText(optionLabelText);
const secondSelectedOption = getByLabelText(oppositeOptionLabelText);

await pressOption(optionLabelText);

expect(firstSelectedOption).toHaveAttribute("checked");

await pressOption(oppositeOptionLabelText);

expect(firstSelectedOption).not.toHaveAttribute("checked");
expect(secondSelectedOption).toHaveAttribute("checked");
});
},
);

describe("when an exclusiveOr option is configured", () => {
it("does not affect the user's ability to select multiple other options", async () => {
const handleSubmit = vi.fn();

setup(
<Checklist
allRequired={false}
description=""
text="Which permanent structures have you lived in?"
handleSubmit={handleSubmit}
options={optionsWithExclusiveOption}
/>,
);
expect(screen.getByRole("heading")).toHaveTextContent(
"Which permanent structures have you lived in?",
);

await pressOption("Caravan");
await pressOption("House");

await pressContinue();

expect(handleSubmit).toHaveBeenCalledWith({
answers: ["caravan_id", "house_id"],
});
});
});
Loading
Loading