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.

Sign up for GitHub

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(a11y): Remove disabled button from Question component #3149

Merged
merged 3 commits into from
May 16, 2024
Merged

feat(a11y): Remove disabled button from Question component #3149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b7e466
feat(a11y): Remove disabled button from question component
DafyddLlyr May 15, 2024
ece17bd
test: Fix tests
DafyddLlyr May 15, 2024
e0dadb7
fix: Error wrapper text
DafyddLlyr May 16, 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
161 changes: 92 additions & 69 deletions editor.planx.uk/src/@planx/components/Question/Public.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,75 +52,98 @@ describe("Question component", () => {
QuestionLayout.Images,
QuestionLayout.Descriptions,
].forEach((type) => {
it(`renders the ${QuestionLayout[type]} layout correctly`, async () => {
const handleSubmit = jest.fn();

const { user } = setup(
<Question
text="Best food"
responses={responses[type]}
handleSubmit={handleSubmit}
/>,
);

const continueButton = screen.getByTestId("continue-button");

expect(screen.getByRole("heading")).toHaveTextContent("Best food");

expect(continueButton).toBeDisabled();

await user.click(screen.getByText("Pizza"));

expect(continueButton).not.toBeDisabled();

await user.click(continueButton);

await waitFor(() =>
expect(handleSubmit).toHaveBeenCalledWith({ answers: ["pizza_id"] }),
);
});

it(`should display previously selected answer on back or change in the ${QuestionLayout[type]} layout`, async () => {
const handleSubmit = jest.fn();
const { user } = setup(
<Question
text="Best food"
responses={responses[type]}
previouslySubmittedData={{
answers: ["celery_id"],
auto: false,
}}
handleSubmit={handleSubmit}
/>,
);

expect(screen.getByRole("heading")).toHaveTextContent("Best food");

const celeryRadio = screen.getByRole("radio", { name: /Celery/ });
const pizzaRadio = screen.getByRole("radio", { name: /Pizza/ });

// State is preserved...
expect(celeryRadio).toBeChecked();

// ...and can be updated
await user.click(pizzaRadio);
expect(pizzaRadio).toBeChecked();

const continueButton = screen.getByTestId("continue-button");
expect(continueButton).toBeEnabled();
});

it(`should not have any accessibility violations in the ${QuestionLayout[type]} layout`, async () => {
const handleSubmit = jest.fn();
const { container } = setup(
<Question
text="Best food"
responses={responses[type]}
handleSubmit={handleSubmit}
/>,
);
const results = await axe(container);
expect(results).toHaveNoViolations();
describe(`${QuestionLayout[type]} layout`, () => {
it(`renders the layout correctly`, async () => {
const handleSubmit = jest.fn();

const { user, getByTestId, getByRole, getByText } = setup(
<Question
text="Best food"
responses={responses[type]}
handleSubmit={handleSubmit}
/>,
);

const continueButton = getByTestId("continue-button");

expect(getByRole("heading")).toHaveTextContent("Best food");

await user.click(getByText("Pizza"));

await user.click(continueButton);

await waitFor(() =>
expect(handleSubmit).toHaveBeenCalledWith({ answers: ["pizza_id"] }),
);
});

it(`should display previously selected answer on back or change`, async () => {
const handleSubmit = jest.fn();
const { user, getByRole, getByTestId } = setup(
<Question
text="Best food"
responses={responses[type]}
previouslySubmittedData={{
answers: ["celery_id"],
auto: false,
}}
handleSubmit={handleSubmit}
/>,
);

expect(getByRole("heading")).toHaveTextContent("Best food");

const celeryRadio = getByRole("radio", { name: /Celery/ });
const pizzaRadio = getByRole("radio", { name: /Pizza/ });

// State is preserved...
expect(celeryRadio).toBeChecked();

// ...and can be updated
await user.click(pizzaRadio);
expect(pizzaRadio).toBeChecked();

const continueButton = getByTestId("continue-button");
expect(continueButton).toBeEnabled();
});

it(`should not have any accessibility violations`, async () => {
const handleSubmit = jest.fn();
const { container } = setup(
<Question
text="Best food"
responses={responses[type]}
handleSubmit={handleSubmit}
/>,
);
const results = await axe(container);
expect(results).toHaveNoViolations();
});

it(`should display an error message if no option is selected`, async () => {
const handleSubmit = jest.fn();
const errorMessage = /Select your answer before continuing/;

const { user, getByTestId, getByText, queryByText } = setup(
<Question
text="Best food"
responses={responses[type]}
handleSubmit={handleSubmit}
/>,
);

const continueButton = getByTestId("continue-button");

expect(continueButton).not.toBeDisabled();
expect(queryByText(errorMessage)).not.toBeInTheDocument();

await user.click(continueButton);

await waitFor(() => {
expect(handleSubmit).not.toHaveBeenCalled();
expect(getByText(errorMessage)).toBeVisible();
});
});
});
});
});
143 changes: 74 additions & 69 deletions editor.planx.uk/src/@planx/components/Question/Public/Question.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import { handleSubmit } from "pages/Preview/Node";
import React from "react";
import FormWrapper from "ui/public/FormWrapper";
import FullWidthWrapper from "ui/public/FullWidthWrapper";
import ErrorWrapper from "ui/shared/ErrorWrapper";
import { mixed, object, string } from "yup";

export interface IQuestion {
id?: string;
Expand Down Expand Up @@ -57,15 +59,17 @@ const Question: React.FC<IQuestion> = (props) => {
a: previousResponseKey ?? undefined,
},
},
onSubmit: (values) => {
setTimeout(
() => props.handleSubmit({ answers: [values.selected.id] }),
theme.transitions.duration.standard,
);
},
validate: () => {
// do nothing
},
onSubmit: (values) => props.handleSubmit({ answers: [values.selected.id] }),
validateOnBlur: false,
validateOnChange: false,
validationSchema: object({
selected: object({
id: string().required("Select your answer before continuing"),
a: mixed().required().test(value =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selected.a can be undefined in initialValues - is it actually required here or optional?

Also nice to see the mixed() syntax in action 💪

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I think what I have is correct, as this is a required value at time of submission.

When the component loads the value is not set as it doesn't exist (so it's optional in the props). When navigating "back" it will be set already. The value for selected.a is set alongside selected.id here -

planx-new/editor.planx.uk/src/@planx/components/Question/Public/Question.tsx

Lines 115 to 116 in 6193731

formik.setFieldValue("selected.id", response.id);
formik.setFieldValue("selected.a", response.responseKey);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That distinction makes sense - thanks for clarifying!

typeof value === "number" ||
typeof value === "string")
})
}),
});

let layout = QuestionLayout.Basic;
Expand All @@ -80,7 +84,6 @@ const Question: React.FC<IQuestion> = (props) => {
return (
<Card
handleSubmit={formik.handleSubmit}
isValid={Boolean(formik.values.selected.id)}
>
<QuestionHeader
title={props.text}
Expand All @@ -100,68 +103,70 @@ const Question: React.FC<IQuestion> = (props) => {
>
{props.text}
</FormLabel>
<RadioGroup
aria-labelledby={`radio-buttons-group-label-${props.id}`}
name={`radio-buttons-group-${props.id}`}
value={formik.values.selected.id}
>
<Grid
container
spacing={layout === QuestionLayout.Basic ? 0 : 2}
alignItems="stretch"
<ErrorWrapper id={props.id} error={formik.errors.selected?.id}>
<RadioGroup
aria-labelledby={`radio-buttons-group-label-${props.id}`}
name={`radio-buttons-group-${props.id}`}
value={formik.values.selected.id}
>
{props.responses?.map((response) => {
const onChange = () => {
formik.setFieldValue("selected.id", response.id);
formik.setFieldValue("selected.a", response.responseKey);
};
const buttonProps = {
onChange,
};
<Grid
container
spacing={layout === QuestionLayout.Basic ? 0 : 2}
alignItems="stretch"
>
{props.responses?.map((response) => {
const onChange = () => {
formik.setFieldValue("selected.id", response.id);
formik.setFieldValue("selected.a", response.responseKey);
};
const buttonProps = {
onChange,
};

switch (layout) {
case QuestionLayout.Basic:
return (
<FormWrapper key={`wrapper-${response.id}`}>
<Grid item xs={12} ml={1} key={`grid-${response.id}`}>
<BasicRadio
{...buttonProps}
{...response}
data-testid="basic-radio"
key={`basic-radio-${response.id}`}
/>
switch (layout) {
case QuestionLayout.Basic:
return (
<FormWrapper key={`wrapper-${response.id}`}>
<Grid item xs={12} ml={1} key={`grid-${response.id}`}>
<BasicRadio
{...buttonProps}
{...response}
data-testid="basic-radio"
key={`basic-radio-${response.id}`}
/>
</Grid>
</FormWrapper>
);
case QuestionLayout.Descriptions:
return (
<Grid
item
xs={12}
sm={6}
contentWrap={4}
key={response.id}
data-testid="description-radio"
>
<DescriptionRadio {...buttonProps} {...response} />
</Grid>
);
case QuestionLayout.Images:
return (
<Grid
item
xs={12}
sm={6}
contentWrap={4}
key={response.id}
>
<ImageRadio {...buttonProps} {...response} />
</Grid>
</FormWrapper>
);
case QuestionLayout.Descriptions:
return (
<Grid
item
xs={12}
sm={6}
contentWrap={4}
key={response.id}
data-testid="description-radio"
>
<DescriptionRadio {...buttonProps} {...response} />
</Grid>
);
case QuestionLayout.Images:
return (
<Grid
item
xs={12}
sm={6}
contentWrap={4}
key={response.id}
>
<ImageRadio {...buttonProps} {...response} />
</Grid>
);
}
})}
</Grid>
</RadioGroup>
);
}
})}
</Grid>
</RadioGroup>
</ErrorWrapper>
</FormControl>
</FullWidthWrapper>
</Card>
Expand Down
Loading