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: Add error handling for failed file uploads #3369

Merged
merged 1 commit into from
Jul 8, 2024
Merged

feat: Add error handling for failed file uploads #3369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions editor.planx.uk/src/@planx/components/FileUpload/Public.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,17 @@ const slotsSchema = array()
!slots.some((slot) => slot.status === "uploading"),
);
},
})
.test({
name: "errorStatus",
message: "Remove files which failed to upload",
test: (slots?: Array<FileUploadSlot>) => {
return Boolean(
slots &&
slots.length > 0 &&
!slots.some((slot) => slot.status === "error"),
);
},
});

const FileUpload: React.FC<Props> = (props) => {
Expand Down
5 changes: 1 addition & 4 deletions editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ function Component(props: Props) {
break;
}
case "allRequiredFilesUploaded":
case "errorStatus":
setFileListError(err?.message);
break;
}
Expand Down Expand Up @@ -188,10 +189,6 @@ function Component(props: Props) {
return (
<Card
handleSubmit={props.hideDropZone ? props.handleSubmit : validateAndSubmit}
isValid={
props.hideDropZone ||
slots.every((slot) => slot.url && slot.status === "success")
}
Comment on lines -191 to -194
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ianjon3s Having a condition here on the isValid prop means that the "Continue" button is disabled, which goes against our general a11y principles.

Did we ever discuss this one, or is removing this here the right way to go?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch, I think I missed this one when inventoring / fixing disabled "Continue" buttons here if you want to add to existing Trello checklist for collective memory! https://trello.com/c/elmTB1Pq/2828-disabled-buttons-and-unexpected-input-functionality-usability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've actually removed it in this PR - added to checklist and marked as done!

>
<FullWidthWrapper>
<CardHeader {...props} />
Expand Down
17 changes: 14 additions & 3 deletions editor.planx.uk/src/@planx/components/FileUploadAndLabel/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,21 @@ export const slotsSchema = array()
name: "nonUploading",
message: "Please wait for upload to complete",
test: (slots?: Array<FileUploadSlot>) => {
const isEveryUploadComplete = Boolean(
slots?.every((slot) => slot.status === "success"),
const isAnyUploadInProgress = Boolean(
slots?.some((slot) => slot.status === "uploading"),
);
return !isAnyUploadInProgress;
},
})
.test({
name: "errorStatus",
message: "Remove files which failed to upload",
test: (slots?: Array<FileUploadSlot>) => {
return Boolean(
slots &&
slots.length > 0 &&
!slots.some((slot) => slot.status === "error"),
);
return isEveryUploadComplete;
},
});

Expand Down
144 changes: 80 additions & 64 deletions editor.planx.uk/src/@planx/components/shared/PrivateFileUpload/UploadedFileCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { visuallyHidden } from "@mui/utils";
import { FileUploadSlot } from "@planx/components/FileUpload/Public";
import ImagePreview from "components/ImagePreview";
import React from "react";
import ErrorWrapper from "ui/shared/ErrorWrapper";

interface Props extends FileUploadSlot {
removeFile: () => void;
Expand All @@ -19,12 +20,12 @@ interface Props extends FileUploadSlot {
}

const Root = styled(Box)(({ theme }) => ({
border: `1px solid ${theme.palette.border.main}`,
marginBottom: theme.spacing(0.5),
marginTop: theme.spacing(5),
}));

const FileCard = styled(Box)(({ theme }) => ({
border: `1px solid ${theme.palette.border.main}`,
position: "relative",
height: "auto",
display: "flex",
Expand Down Expand Up @@ -91,74 +92,89 @@ export const UploadedFileCard: React.FC<Props> = ({
removeFile,
onChange,
tags,
status,
}) => (
<Root>
<FileCard>
<ProgressBar
width={`${Math.min(Math.ceil(progress * 100), 100)}%`}
role="progressbar"
aria-valuenow={progress * 100 || 0}
aria-label={file.path}
/>
<FilePreview>
{file instanceof File && file?.type?.includes("image") ? (
<ImagePreview file={file} url={url} />
) : (
<FileIcon />
)}
</FilePreview>
<Box
sx={{ display: "flex", justifyContent: "space-between", width: "100%" }}
>
<Box mr={2}>
<Typography
variant="body1"
pb="0.25em"
sx={{ overflowWrap: "break-word", wordBreak: "break-all" }}
<ErrorWrapper
error={
status === "error"
? "Upload failed, please remove file and try again"
: undefined
}
>
Comment on lines +98 to +104
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this wrapper is the change here, the diff below is purely a whitespace change due to the extra indentation but git hasn't quite caught this.

<>
<FileCard>
<ProgressBar
width={`${Math.min(Math.ceil(progress * 100), 100)}%`}
role="progressbar"
aria-valuenow={progress * 100 || 0}
aria-label={file.path}
/>
<FilePreview>
{file instanceof File && file?.type?.includes("image") ? (
<ImagePreview file={file} url={url} />
) : (
<FileIcon />
)}
</FilePreview>
<Box
sx={{
display: "flex",
justifyContent: "space-between",
width: "100%",
}}
>
{file.path}
</Typography>
<FileSize variant="body2">{formatBytes(file.size)}</FileSize>
</Box>
{removeFile && (
<IconButton
size="small"
aria-label={`Delete ${file.path}`}
title={`Delete ${file.path}`}
onClick={removeFile}
>
<DeleteIcon />
</IconButton>
)}
</Box>
</FileCard>
{tags && (
<TagRoot>
<Box sx={{ display: "flex", flexWrap: "wrap", gap: 1 }}>
{tags.map((tag) => (
<ListItem key={tag} disablePadding sx={{ width: "auto" }}>
<Chip
label={tag}
variant="uploadedFileTag"
<Box mr={2}>
<Typography
variant="body1"
pb="0.25em"
sx={{ overflowWrap: "break-word", wordBreak: "break-all" }}
>
{file.path}
</Typography>
<FileSize variant="body2">{formatBytes(file.size)}</FileSize>
</Box>
{removeFile && (
<IconButton
size="small"
data-testid="uploaded-file-chip"
/>
</ListItem>
))}
</Box>
<Link
onClick={() => onChange && onChange()}
sx={{ fontFamily: "inherit", fontSize: "inherit" }}
component="button"
variant="body2"
>
Change
<Box sx={visuallyHidden} component="span">
the list of what file {file.path} shows
aria-label={`Delete ${file.path}`}
title={`Delete ${file.path}`}
onClick={removeFile}
>
<DeleteIcon />
</IconButton>
)}
</Box>
</Link>
</TagRoot>
)}
</FileCard>
{tags && (
<TagRoot>
<Box sx={{ display: "flex", flexWrap: "wrap", gap: 1 }}>
{tags.map((tag) => (
<ListItem key={tag} disablePadding sx={{ width: "auto" }}>
<Chip
label={tag}
variant="uploadedFileTag"
size="small"
data-testid="uploaded-file-chip"
/>
</ListItem>
))}
</Box>
<Link
onClick={() => onChange && onChange()}
sx={{ fontFamily: "inherit", fontSize: "inherit" }}
component="button"
variant="body2"
>
Change
<Box sx={visuallyHidden} component="span">
the list of what file {file.path} shows
</Box>
</Link>
</TagRoot>
)}
</>
</ErrorWrapper>
</Root>
);

Expand Down
Loading