Skip to content

Commit

Permalink
fix(a11y): FileUploadAndLabel error messages (#2973)
Browse files Browse the repository at this point in the history
  • Loading branch information
DafyddLlyr authored Apr 3, 2024
1 parent 1326e82 commit 5b642e6
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 63 deletions.
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

0 comments on commit 5b642e6

Please sign in to comment.