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

proposal: image-upload component #1791

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

proposal: image-upload component #1791

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91fffe8
create image-upload component
ajhollid Feb 19, 2025
64db414
generalize error
ajhollid Feb 19, 2025
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
176 changes: 176 additions & 0 deletions src/Components/Inputs/ImageUpload/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
// Components
import { Button, Box, Stack, Typography } from "@mui/material";
import CloudUploadIcon from "@mui/icons-material/CloudUpload";
import Image from "../../../Components/Image";

// Utils
import PropTypes from "prop-types";
import { createToast } from "../../../Utils/toastUtils";
import { formatBytes } from "../../../Utils/fileUtils";
import { useCallback } from "react";
import { useTheme } from "@emotion/react";

/**
* ImageUpload component allows users to upload images with drag-and-drop functionality.
* It supports file size and format validation.
*
* @component
* @param {Object} props - Component props
* @param {boolean} [props.previewIsRound=false] - Determines if the image preview should be round
* @param {string} [props.src] - Source URL of the image to display
* @param {function} props.onChange - Callback function to handle file change, takes a file as an argument
* @param {number} [props.maxSize=3145728] - Maximum file size allowed in bytes (default is 3MB)
* @param {Array<string>} [props.accept=['jpg', 'jpeg', 'png']] - Array of accepted file formats
* @param {Object} [props.errors] - Object containing error messages
* @returns {JSX.Element} The rendered component
*/
const ImageUpload = ({
previewIsRound = false,
src,
onChange,
maxSize = 3 * 1024 * 1024,
accept = ["jpg", "jpeg", "png"],
errors,
}) => {
const theme = useTheme();
const roundStyle = previewIsRound ? { borderRadius: "50%" } : {};

const handleImageChange = useCallback(
(file) => {
if (file.size > maxSize) {
createToast({
body: "File size is too large",
});
return;
}
onChange(file);
},
[maxSize, onChange]
);
Comment on lines +38 to +49
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Secure this spaghetti code, fam!

The file handling needs some extra sauce:

  • Add file type validation
  • Check for null/undefined files
  • Consider adding virus scanning integration
 const handleImageChange = useCallback(
   (file) => {
+    if (!file) return;
+    
+    const fileType = file.type.split('/')[1];
+    if (!accept.includes(fileType)) {
+      createToast({
+        body: `Invalid file type. Supported formats: ${accept.join(', ')}`,
+      });
+      return;
+    }
+
     if (file.size > maxSize) {
       createToast({
         body: "File size is too large",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const handleImageChange = useCallback(
(file) => {
if (file.size > maxSize) {
createToast({
body: "File size is too large",
});
return;
}
onChange(file);
},
[maxSize, onChange]
);
const handleImageChange = useCallback(
(file) => {
if (!file) return;
const fileType = file.type.split('/')[1];
if (!accept.includes(fileType)) {
createToast({
body: `Invalid file type. Supported formats: ${accept.join(', ')}`,
});
return;
}
if (file.size > maxSize) {
createToast({
body: "File size is too large",
});
return;
}
onChange(file);
},
[maxSize, onChange]
);


if (src) {
return (
<Stack alignItems="center">
<Image
alt="company logo"
maxWidth="250px"
maxHeight="250px"
src={src}
sx={{ ...roundStyle }}
/>
</Stack>
);
}

const hasError = errors?.logo;

return (
<>
<Box
minHeight={175}
sx={{
position: "relative",
border: "dashed",
borderRadius: theme.shape.borderRadius,
borderColor: theme.palette.primary.lowContrast,
borderWidth: "2px",
transition: "0.2s",
"&:hover": {
borderColor: theme.palette.primary.main,
backgroundColor: "hsl(215, 87%, 51%, 0.05)",
},
}}
onDragLeave={(e) => {
e.preventDefault();
}}
onDragOver={(e) => {
e.preventDefault();
}}
onDrop={(e) => {
e.preventDefault();
handleImageChange(e?.dataTransfer?.files?.[0]);
}}
Comment on lines +81 to +90
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Drop it like it's hot, but safely!

The drag and drop handlers need some defensive programming:

  • Add error handling for failed drops
  • Validate file count (only one file allowed)
 onDrop={(e) => {
   e.preventDefault();
-  handleImageChange(e?.dataTransfer?.files?.[0]);
+  try {
+    const files = e?.dataTransfer?.files;
+    if (!files || files.length === 0) {
+      createToast({ body: "No file was dropped" });
+      return;
+    }
+    if (files.length > 1) {
+      createToast({ body: "Please drop only one file" });
+      return;
+    }
+    handleImageChange(files[0]);
+  } catch (error) {
+    createToast({ body: "Failed to process dropped file" });
+  }
 }}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
onDragLeave={(e) => {
e.preventDefault();
}}
onDragOver={(e) => {
e.preventDefault();
}}
onDrop={(e) => {
e.preventDefault();
handleImageChange(e?.dataTransfer?.files?.[0]);
}}
onDragLeave={(e) => {
e.preventDefault();
}}
onDragOver={(e) => {
e.preventDefault();
}}
onDrop={(e) => {
e.preventDefault();
try {
const files = e?.dataTransfer?.files;
if (!files || files.length === 0) {
createToast({ body: "No file was dropped" });
return;
}
if (files.length > 1) {
createToast({ body: "Please drop only one file" });
return;
}
handleImageChange(files[0]);
} catch (error) {
createToast({ body: "Failed to process dropped file" });
}
}}

>
<Button
fullWidth
component="label"
role={undefined}
sx={{
height: "100%",
}}
>
<Stack alignItems="center">
<CloudUploadIcon />
<Typography
component="h2"
color={theme.palette.primary.contrastTextTertiary}
>
<Typography
component="span"
fontSize="inherit"
color="info"
fontWeight={500}
>
Click to upload
</Typography>{" "}
or drag and drop
</Typography>
<Typography
component="p"
color={theme.palette.primary.contrastTextTertiary}
sx={{ opacity: 0.6 }}
>
(maximum size: {formatBytes(maxSize)})
</Typography>
</Stack>
<input
style={{
clip: "rect(0 0 0 0)",
clipPath: "inset(50%)",
height: 1,
overflow: "hidden",
position: "absolute",
bottom: 0,
left: 0,
whiteSpace: "nowrap",
width: 1,
}}
onChange={(e) => handleImageChange(e?.target?.files?.[0])}
type="file"
accept={accept.map((format) => `image/${format}`).join(", ")}
/>
</Button>
Comment on lines +92 to +140
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

There's accessibility on his sweater already!

The upload button needs some a11y love:

  • Add aria-label
  • Improve screen reader experience
  • Consider adding keyboard navigation support
 <Button
   fullWidth
   component="label"
   role={undefined}
+  aria-label="Upload image"
+  aria-describedby="upload-description"
   sx={{
     height: "100%",
   }}
 >
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<Button
fullWidth
component="label"
role={undefined}
sx={{
height: "100%",
}}
>
<Stack alignItems="center">
<CloudUploadIcon />
<Typography
component="h2"
color={theme.palette.primary.contrastTextTertiary}
>
<Typography
component="span"
fontSize="inherit"
color="info"
fontWeight={500}
>
Click to upload
</Typography>{" "}
or drag and drop
</Typography>
<Typography
component="p"
color={theme.palette.primary.contrastTextTertiary}
sx={{ opacity: 0.6 }}
>
(maximum size: {formatBytes(maxSize)})
</Typography>
</Stack>
<input
style={{
clip: "rect(0 0 0 0)",
clipPath: "inset(50%)",
height: 1,
overflow: "hidden",
position: "absolute",
bottom: 0,
left: 0,
whiteSpace: "nowrap",
width: 1,
}}
onChange={(e) => handleImageChange(e?.target?.files?.[0])}
type="file"
accept={accept.map((format) => `image/${format}`).join(", ")}
/>
</Button>
<Button
fullWidth
component="label"
role={undefined}
aria-label="Upload image"
aria-describedby="upload-description"
sx={{
height: "100%",
}}
>
<Stack alignItems="center">
<CloudUploadIcon />
<Typography
component="h2"
color={theme.palette.primary.contrastTextTertiary}
>
<Typography
component="span"
fontSize="inherit"
color="info"
fontWeight={500}
>
Click to upload
</Typography>{" "}
or drag and drop
</Typography>
<Typography
component="p"
color={theme.palette.primary.contrastTextTertiary}
sx={{ opacity: 0.6 }}
>
(maximum size: {formatBytes(maxSize)})
</Typography>
</Stack>
<input
style={{
clip: "rect(0 0 0 0)",
clipPath: "inset(50%)",
height: 1,
overflow: "hidden",
position: "absolute",
bottom: 0,
left: 0,
whiteSpace: "nowrap",
width: 1,
}}
onChange={(e) => handleImageChange(e?.target?.files?.[0])}
type="file"
accept={accept.map((format) => `image/${format}`).join(", ")}
/>
</Button>

</Box>
<Typography
component="p"
sx={{ opacity: 0.6 }}
>
Supported formats: {accept.join(", ").toUpperCase()}
</Typography>
{hasError && (
<Typography
component="span"
className="input-error"
color={theme.palette.error.main}
mt={theme.spacing(2)}
sx={{
opacity: 0.8,
}}
>
{hasError}
</Typography>
)}
</>
);
};

ImageUpload.propTypes = {
previewIsRound: PropTypes.bool,
src: PropTypes.string,
onChange: PropTypes.func,
maxSize: PropTypes.number,
accept: PropTypes.array,
errors: PropTypes.object,
};
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Props types weak, arms are heavy!

The PropTypes need some strengthening:

  • Make onChange required
  • Use arrayOf for accept prop
  • Add shape for errors object
 ImageUpload.propTypes = {
   previewIsRound: PropTypes.bool,
   src: PropTypes.string,
-  onChange: PropTypes.func,
+  onChange: PropTypes.func.isRequired,
-  accept: PropTypes.array,
+  accept: PropTypes.arrayOf(PropTypes.string),
-  errors: PropTypes.object,
+  errors: PropTypes.shape({
+    logo: PropTypes.string
+  }),
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ImageUpload.propTypes = {
previewIsRound: PropTypes.bool,
src: PropTypes.string,
onChange: PropTypes.func,
maxSize: PropTypes.number,
accept: PropTypes.array,
errors: PropTypes.object,
};
ImageUpload.propTypes = {
previewIsRound: PropTypes.bool,
src: PropTypes.string,
onChange: PropTypes.func.isRequired,
maxSize: PropTypes.number,
accept: PropTypes.arrayOf(PropTypes.string),
errors: PropTypes.shape({
logo: PropTypes.string
}),
};


export default ImageUpload;