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

fix(a11y): FileUploadAndLabel error messages #2973

Merged
merged 3 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
78 changes: 43 additions & 35 deletions editor.planx.uk/src/@planx/components/FileUploadAndLabel/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import capitalize from "lodash/capitalize";
import React, { useEffect, useState } from "react";
import { usePrevious } from "react-use";
import ErrorWrapper from "ui/shared/ErrorWrapper";
import { ValidationError } from "yup";

import { FileUploadSlot } from "../FileUpload/Public";
import { UploadedFileCard } from "../shared/PrivateFileUpload/UploadedFileCard";
Expand All @@ -28,7 +29,7 @@ import {
getTagsForSlot,
removeSlots,
} from "./model";
import { fileLabelSchema } from "./schema";
import { fileLabelSchema, formatFileLabelSchemaErrors } from "./schema";

interface FileTaggingModalProps {
uploadedFiles: FileUploadSlot[];
Expand All @@ -55,9 +56,9 @@ export const FileTaggingModal = ({
setShowModal,
removeFile,
}: FileTaggingModalProps) => {
const [error, setError] = useState<string | undefined>();
const [errors, setErrors] = useState<Record<string, string> | undefined>();

const closeModal = (event: any, reason?: string) => {
const closeModal = (_event: any, reason?: string) => {
if (reason && reason == "backdropClick") {
return;
}
Expand All @@ -68,7 +69,12 @@ export const FileTaggingModal = ({
fileLabelSchema
.validate(fileList, { context: { slots: uploadedFiles } })
.then(closeModal)
.catch((err) => setError(err.message));
.catch((err) => {
if (err instanceof ValidationError) {
const formattedErrors = formatFileLabelSchemaErrors(err);
setErrors(formattedErrors);
}
});
};

return (
Expand Down Expand Up @@ -97,16 +103,20 @@ export const FileTaggingModal = ({
</Box>
{uploadedFiles.map((slot) => (
<Box sx={{ mb: 4 }} key={`tags-per-file-container-${slot.id}`}>
<UploadedFileCard
{...slot}
key={slot.id}
removeFile={() => removeFile(slot)}
/>
<SelectMultiple
uploadedFile={slot}
fileList={fileList}
setFileList={setFileList}
/>
<ErrorWrapper error={errors?.[slot.id]}>
<>
<UploadedFileCard
{...slot}
key={slot.id}
removeFile={() => removeFile(slot)}
/>
<SelectMultiple
uploadedFile={slot}
fileList={fileList}
setFileList={setFileList}
/>
</>
</ErrorWrapper>
</Box>
))}
</DialogContent>
Expand All @@ -117,27 +127,25 @@ export const FileTaggingModal = ({
padding: 2,
}}
>
<ErrorWrapper error={error}>
<Box>
<Button
variant="contained"
color="prompt"
onClick={handleValidation}
data-testid="modal-done-button"
>
Done
</Button>
<Button
variant="contained"
color="secondary"
sx={{ ml: 1.5 }}
onClick={closeModal}
data-testid="modal-cancel-button"
>
Cancel
</Button>
</Box>
</ErrorWrapper>
<Box>
<Button
variant="contained"
color="prompt"
onClick={handleValidation}
data-testid="modal-done-button"
>
Done
</Button>
<Button
variant="contained"
color="secondary"
sx={{ ml: 1.5 }}
onClick={closeModal}
data-testid="modal-cancel-button"
>
Cancel
</Button>
</Box>
</DialogActions>
</Dialog>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ describe("Error handling", () => {
await user.click(submitModalButton);
expect(true).toBeTruthy();
const modalError = await within(fileTaggingModal).findByText(
"Please label all files",
/File test.jpg is not labeled/,
);
expect(modalError).toBeVisible();
});
Expand Down Expand Up @@ -594,7 +594,9 @@ describe("Error handling", () => {
// User cannot submit without uploading a file
await user.click(screen.getByTestId("continue-button"));
expect(handleSubmit).not.toHaveBeenCalled();
const fileListError = await screen.findByText("Please label all files");
const fileListError = await screen.findByText(
/File test.jpg is not labeled/,
);
expect(fileListError).toBeVisible();

// Re-open modal and tag file
Expand Down
63 changes: 48 additions & 15 deletions editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,12 @@ import {
getTagsForSlot,
removeSlots,
} from "./model";
import { fileLabelSchema, fileListSchema, slotsSchema } from "./schema";
import {
fileLabelSchema,
fileListSchema,
formatFileLabelSchemaErrors,
slotsSchema,
} from "./schema";

type Props = PublicProps<FileUploadAndLabel>;

Expand Down Expand Up @@ -106,6 +111,7 @@ function Component(props: Props) {
if (isUserReturningToNode) return setIsUserReturningToNode(false);
if (slots.length && dropzoneError) setDropzoneError(undefined);
if (!slots.length && fileListError) setFileListError(undefined);
if (!slots.length && fileLabelErrors) setFileLabelErrors(undefined);
if (slots.length > previousSlotCount) setShowModal(true);
}, [slots.length]);

Expand All @@ -114,6 +120,9 @@ function Component(props: Props) {
);

const [dropzoneError, setDropzoneError] = useState<string | undefined>();
const [fileLabelErrors, setFileLabelErrors] = useState<
Record<string, string> | undefined
>();
const [fileListError, setFileListError] = useState<string | undefined>();
const [showModal, setShowModal] = useState<boolean>(false);
const [isUserReturningToNode, setIsUserReturningToNode] =
Expand All @@ -129,15 +138,27 @@ function Component(props: Props) {
const payload = generatePayload(fileList);
props.handleSubmit?.(payload);
})
.catch((err) =>
err?.type === "minFileUploaded"
? setDropzoneError(err?.message)
: setFileListError(err?.message),
);
.catch((err) => {
switch (err?.type) {
case "minFileUploaded":
case "nonUploading":
setDropzoneError(err?.message);
break;
case "allFilesTagged": {
const formattedErrors = formatFileLabelSchemaErrors(err);
setFileLabelErrors(formattedErrors);
break;
}
case "allRequiredFilesUploaded":
setFileListError(err?.message);
break;
}
});
};

const onUploadedFileCardChange = () => {
setFileListError(undefined);
setFileLabelErrors(undefined);
setShowModal(true);
};

Expand Down Expand Up @@ -238,13 +259,15 @@ function Component(props: Props) {
)}
{slots.map((slot) => {
return (
<UploadedFileCard
{...slot}
key={slot.id}
tags={getTagsForSlot(slot.id, fileList)}
onChange={onUploadedFileCardChange}
removeFile={() => removeFile(slot)}
/>
<ErrorWrapper error={fileLabelErrors?.[slot.id]} id={slot.id}>
<UploadedFileCard
{...slot}
key={slot.id}
tags={getTagsForSlot(slot.id, fileList)}
onChange={onUploadedFileCardChange}
removeFile={() => removeFile(slot)}
/>
</ErrorWrapper>
);
})}
</Box>
Expand Down Expand Up @@ -322,8 +345,18 @@ const InteractiveFileListItem = (props: FileListItemProps) => {
{howMeasured && howMeasured !== emptyContent ? (
<MoreInfoSection title="How is it defined?">
<>
{definitionImg && <Image src={definitionImg} alt="" aria-describedby="howMeasured" />}
<ReactMarkdownOrHtml source={howMeasured} openLinksOnNewTab id="howMeasured"/>
{definitionImg && (
<Image
src={definitionImg}
alt=""
aria-describedby="howMeasured"
/>
)}
<ReactMarkdownOrHtml
source={howMeasured}
openLinksOnNewTab
id="howMeasured"
/>
</>
</MoreInfoSection>
) : undefined}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,20 +212,26 @@ describe("fileLabelSchema", () => {
});

it("rejects if any slots are untagged", async () => {
const mockSlots = [{ id: "123" }, { id: "456" }] as FileUploadSlot[];
const mockSlots = [
{ id: "123", file: { path: "first.jpg" } },
{ id: "456", file: { path: "second.jpg" } },
] as FileUploadSlot[];

const mockFileList = {
required: [
// Second slot is not assigned to any fileTypes
{ ...mockFileTypes.AlwaysRequired, slots: [{ id: "123" }] },
{
...mockFileTypes.AlwaysRequired,
slots: [{ id: "123", file: { path: "abc.jpg" } }],
},
],
recommended: [],
optional: [],
};

await expect(() =>
fileLabelSchema.validate(mockFileList, { context: { slots: mockSlots } }),
).rejects.toThrow(/Please label all files/);
).rejects.toThrow(/File second.jpg is not labeled/);
});

it("allows fileLists where all files are tagged, but requirements are not satisfied yet", async () => {
Expand Down
38 changes: 30 additions & 8 deletions editor.planx.uk/src/@planx/components/FileUploadAndLabel/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
SchemaOf,
string,
TestContext,
ValidationError,
} from "yup";

import { FileUploadSlot } from "../FileUpload/Public";
Expand Down Expand Up @@ -103,20 +104,41 @@ interface FileListSchemaTestContext extends TestContext {

export const fileLabelSchema = object().test({
name: "allFilesTagged",
message: "Please label all files",
test: (fileList, { options: { context } }) => {
if (!context) throw new Error("Missing context for fileListSchema");
const { slots } = context as FileListSchemaTestContext;
const isEveryFileTagged = Boolean(
slots?.every(
(slot) =>
getTagsForSlot(slot.id, fileList as unknown as FileList).length,
),
);
return isEveryFileTagged;
const errors: ValidationError[] = [];
slots?.forEach((slot) => {
const tagsForSlot = getTagsForSlot(
slot.id,
fileList as unknown as FileList,
);
if (!tagsForSlot.length) {
errors.push(
new ValidationError(
`File ${slot.file.path} is not labeled`,
undefined,
slot.id,
),
);
}
});

// Valid fileLabelSchema
if (!errors.length) return true;

return new ValidationError(errors, undefined, undefined, "allFilesTagged");
},
});

/**
* Format errors generated by fileLabelSchema so that we can access them by path (file id) in the UI
*/
export const formatFileLabelSchemaErrors = (
errors: ValidationError,
): Record<string, string> =>
Object.fromEntries(errors.inner.map(({ path, message }) => [path, message]));

export const fileListSchema = object({
required: array().test({
name: "allRequiredFilesUploaded",
Expand Down
Loading