-
Notifications
You must be signed in to change notification settings - Fork 234
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
Refactor/fe/image upload component #1737
base: develop
Are you sure you want to change the base?
Changes from 20 commits
2f80a9a
2670375
edd98bf
223cad0
41733d2
e9afb90
8c3c23e
81de3b4
29d142c
d5ecf87
b2237e6
c8b1fb7
05854f5
c3f3353
419d982
b3599c1
dfcfee4
5b97b19
eedfdd3
808cc92
3917b8b
1368ffa
43be0a7
d4d78e8
28c4215
7657cd5
ea18f50
a4f54f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,290 @@ | ||
import { useState, useRef } from "react"; | ||
import { Box, Button, Stack, IconButton, TextField , Typography } from "@mui/material"; | ||
import ProgressUpload from "../ProgressBars"; | ||
import { formatBytes } from "../../Utils/fileUtils"; | ||
import { imageValidation } from "../../Validation/validation"; | ||
import ImageIcon from "@mui/icons-material/Image"; | ||
import {GenericDialog} from "../Dialog/genericDialog"; | ||
import CloudUploadIcon from "@mui/icons-material/CloudUpload"; | ||
import { checkImage } from "../../Utils/fileUtils"; | ||
import "./index.css"; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please include JSdoc for the component, see other components for examples There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please leave comments unresolved so other reviewers can see what's been happening and changes made 👍 |
||
const isValidBase64Image = (data) => { | ||
return /^[A-Za-z0-9+/=]+$/.test(data); | ||
}; | ||
|
||
const ImageUpload = ({ open, onClose, onUpdate, value, currentImage = value, theme, shouldRender = true, placeholder, maxSize, acceptedTypes, previewSize = 150, onError,}) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Theme doesn't need to be a prop, you can use the This is also an exceptionally complicated API 😂 Can we look at simplifying this? I don't think we need to take Also feel free to create component variants as well to keep the API simple.
Something along those lines. The more complexity we can hide the better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now using the I'm still Exploring with the component variants. Are there specific use cases you had in mind? |
||
|
||
const [file, setFile] = useState(); | ||
const [progress, setProgress] = useState({ value: 0, isLoading: false }); | ||
const [errors, setErrors] = useState({}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it makes sense to handle errors internally as this component will almost certainly be used in a form which will need errors set on it. Probably the best thing to do is accept a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the component to accept |
||
const [isDragging, setIsDragging] = useState(false); | ||
const intervalRef = useRef(null); | ||
|
||
// Handle base64 and placeholder logic | ||
let imageSrc = currentImage; | ||
|
||
if (typeof file?.src !== "undefined") { | ||
imageSrc = file.src; | ||
} else if (typeof currentImage !== "undefined" && isValidBase64Image(currentImage)) { | ||
imageSrc = `data:image/png;base64,${currentImage}`; | ||
} else if (typeof placeholder !== "undefined") { | ||
imageSrc = placeholder; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This value is never used? |
||
} | ||
|
||
if (shouldRender === false) { | ||
return null; | ||
} | ||
|
||
const handleDragEnter = (event) => { | ||
event.preventDefault(); | ||
setIsDragging(true); | ||
}; | ||
|
||
const handleDragLeave = (event) => { | ||
event.preventDefault(); | ||
setIsDragging(false); | ||
}; | ||
|
||
const handleDrop = (event) => { | ||
event.preventDefault(); | ||
setIsDragging(false); | ||
|
||
if (event.dataTransfer.files.length > 0) { | ||
handlePicture({ target: { files: event.dataTransfer.files } }); | ||
} | ||
}; | ||
|
||
// Handles image file selection | ||
const handlePicture = (event) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Name can be a bit confusing here. The parameter expects an event, but we are passing an object that contains part of the event, I think we should rename that |
||
const pic = event.target.files[0]; | ||
if (!pic) return; | ||
|
||
event.target.value = ""; | ||
setFile(null); | ||
|
||
if (maxSize && pic.size > maxSize) { | ||
const errorMsg = `File size exceeds ${formatBytes(maxSize)}`; | ||
setErrors({ picture: errorMsg }); | ||
if (onError) onError(errorMsg); | ||
return; | ||
} | ||
|
||
if (acceptedTypes && !acceptedTypes.includes(pic.type)) { | ||
const errorMsg = `File type not supported. Allowed: ${acceptedTypes.join(", ")}`; | ||
setErrors({ picture: errorMsg }); | ||
if (onError) onError(errorMsg); | ||
return; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, I like the early returns. |
||
// Validate file type and size using schema | ||
let error = validateField({ type: pic.type, size: pic.size }, imageValidation); | ||
if (error) return; | ||
|
||
setProgress({ value: 0, isLoading: true }); | ||
setFile({ | ||
src: URL.createObjectURL(pic), | ||
name: pic.name, | ||
size: formatBytes(pic.size), | ||
delete: false, | ||
}); | ||
|
||
// Simulate upload progress | ||
intervalRef.current = setInterval(() => { | ||
const buffer = 12; | ||
setProgress((prev) => { | ||
if (prev.value + buffer >= 100) { | ||
clearInterval(intervalRef.current); | ||
return { value: 100, isLoading: false }; | ||
} | ||
return { ...prev, value: prev.value + buffer }; | ||
}); | ||
}, 120); | ||
vishnusn77 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
// Validates input against provided schema and updates error state | ||
const validateField = (toValidate, schema, name = "picture") => { | ||
const { error } = schema.validate(toValidate, { abortEarly: false }); | ||
setErrors((prev) => { | ||
const prevErrors = { ...prev }; | ||
if (error) { | ||
prevErrors[name] = error.details[0].message; | ||
if (onError) onError(error.details[0].message); | ||
} else { | ||
delete prevErrors[name]; | ||
} | ||
return prevErrors; | ||
}); | ||
return !!error; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are the possibilities for the error returned? |
||
}; | ||
|
||
// Resets picture-related states and clears interval | ||
const removePicture = () => { | ||
errors["picture"] && setErrors((prev) => ({ ...prev, picture: undefined })); | ||
setFile(null); | ||
clearInterval(intervalRef.current); | ||
setProgress({ value: 0, isLoading: false }); | ||
}; | ||
|
||
// Updates the profile picture and closes the modal | ||
const handleUpdatePicture = () => { | ||
setProgress({ value: 0, isLoading: false }); | ||
onUpdate(file.src); // Pass the new image URL to the parent component | ||
onClose(); // Close the modal | ||
}; | ||
|
||
return ( | ||
<GenericDialog | ||
id="modal-update-picture" | ||
open={open} | ||
onClose={onClose} | ||
theme={theme} | ||
title={"Upload Image"} | ||
description={"Select an image to upload."} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strings should not be hardcoded anymore as we are moving towards providing localization. Please see the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replaced hardcoded strings with i18n |
||
confirmationButtonLabel={"Update"} | ||
onConfirm={handleUpdatePicture} | ||
isLoading={false} | ||
> | ||
<Box | ||
className="image-field-wrapper" | ||
sx={{ | ||
position: "relative", | ||
display: "flex", | ||
flexDirection: "column", | ||
alignItems: "center", | ||
justifyContent: "center", | ||
minHeight: "180px", | ||
maxHeight: "220px", | ||
border: "dashed", | ||
borderRadius: theme.shape.borderRadius, | ||
borderColor: isDragging ? theme.palette.primary.main : theme.palette.primary.lowContrast, | ||
borderWidth: "2px", | ||
transition: "0.2s", | ||
"&:hover": { | ||
borderColor: theme.palette.primary.main, | ||
backgroundColor: "hsl(215, 87%, 51%, 0.05)", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Colors, heights, and borderWidth/radius should not be hardcoded. Please touch base with @marcelluscaio on colors and widths. |
||
}, | ||
}} | ||
onDragEnter={handleDragEnter} | ||
onDragLeave={handleDragLeave} | ||
onDrop={handleDrop} | ||
onDragOver={(e) => e.preventDefault()} | ||
> | ||
<input | ||
id="update-profile-picture" | ||
type="file" | ||
onChange={handlePicture} | ||
style={{ | ||
position: "absolute", | ||
top: 0, | ||
left: 0, | ||
width: "100%", | ||
height: "100%", | ||
opacity: 0, | ||
cursor: "pointer", | ||
zIndex: 3, | ||
pointerEvents: "all", | ||
}} | ||
/> | ||
Comment on lines
+190
to
+205
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to use MUIs Input component? |
||
|
||
{!checkImage(file?.src || currentImage) || progress.isLoading ? ( | ||
<> | ||
<Stack | ||
className="custom-file-text" | ||
alignItems="center" | ||
gap="4px" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use |
||
sx={{ | ||
position: "absolute", | ||
top: "50%", | ||
left: "50%", | ||
transform: "translate(-50%, -50%)", | ||
zIndex: 1, | ||
width: "100%", | ||
}} | ||
> | ||
<IconButton | ||
sx={{ | ||
pointerEvents: "none", | ||
borderRadius: theme.shape.borderRadius, | ||
border: `solid ${theme.shape.borderThick}px ${theme.palette.primary.lowContrast}`, | ||
boxShadow: theme.shape.boxShadow, | ||
}} | ||
> | ||
<CloudUploadIcon /> | ||
</IconButton> | ||
<Typography component="h2" color={theme.palette.primary.contrastTextTertiary}> | ||
<Typography component="span" fontSize="inherit" color="info" fontWeight={500}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fontWeight should not be hardcoded, please use the theme. @marcelluscaio is the most knowledgeable on the theme so please reach out There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure we should be using color info, will take a look into that |
||
Click to upload | ||
</Typography>{" "} | ||
or drag and drop | ||
</Typography> | ||
<Typography | ||
component="p" | ||
color={theme.palette.primary.contrastTextTertiary} | ||
sx={{ opacity: 0.6 }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have we tested if this passes contrast tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can guide devs on how to do contrast tests - maybe a link or similar? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely! https://webaim.org/resources/contrastchecker/ However the first one doesn't support opacity changes. So basically you put the hex values and change the alpha to 0.6, which is the opacity being used here |
||
> | ||
(maximum size: 3MB) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be configurable by prop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved |
||
</Typography> | ||
</Stack> | ||
</> | ||
) : ( | ||
<Box | ||
sx={{ | ||
width: `${previewSize}px`, | ||
height: `${previewSize}px`, | ||
overflow: "hidden", | ||
backgroundImage: `url(${file?.src || currentImage})`, | ||
backgroundSize: "cover", | ||
backgroundPosition: "center", | ||
borderRadius: "50%", | ||
display: "flex", | ||
alignItems: "center", | ||
justifyContent: "center", | ||
}} | ||
/> | ||
)} | ||
</Box> | ||
|
||
{progress.isLoading || progress.value !== 0 || errors["picture"] ? ( | ||
<ProgressUpload | ||
icon={<ImageIcon />} | ||
label={file?.name} | ||
size={file?.size} | ||
progress={progress.value} | ||
onClick={removePicture} | ||
error={errors["picture"]} | ||
/> | ||
) : null} | ||
|
||
<Stack | ||
direction="row" | ||
mt={2} | ||
gap={2} | ||
justifyContent="flex-end" | ||
> | ||
<Button | ||
variant="text" | ||
color="info" | ||
onClick={removePicture} | ||
> | ||
Remove | ||
</Button> | ||
<Button | ||
variant="contained" | ||
color="accent" | ||
onClick={handleUpdatePicture} | ||
disabled={ | ||
(Object.keys(errors).length !== 0 && errors?.picture) || | ||
progress.value !== 100 | ||
? true | ||
: false | ||
} | ||
> | ||
Update | ||
</Button> | ||
</Stack> | ||
</GenericDialog> | ||
); | ||
}; | ||
|
||
export default ImageUpload; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be using css for styling, this can be removed