Refactor/fe/image upload component #1737

vishnusn77 · 2025-02-10T23:58:29Z

Describe your changes

Refactored Image upload component to include all the functionality under single component

Issue number

Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.

(Do not skip this or your PR will be closed) I deployed the application locally.
(Do not skip this or your PR will be closed) I have performed a self-review and testing of my code.
I have included the issue # in the PR.
I have labelled the PR correctly.
The issue I am working on is assigned to me.
I didn't use any hardcoded values (otherwise it will not scale, and will make it difficult to maintain consistency across the application).
I made sure font sizes, color choices etc are all referenced from the theme.
My PR is granular and targeted to one specific feature.
I took a screenshot or a video and attached to this PR if there is a UI change.

ajhollid

A good start! There are many hardcoded values that need to be refactored, and the error handling probably should be moved to the parent component.

I realize many of the hardcoded values are from a previous dev, so if you're not sure how to deal with them please reach out to @marcelluscaio , he knows the most about the theme and how it can be modified to refactor out those hardcoded values.

Client/src/Components/ImageUpload/index.jsx

ajhollid · 2025-02-11T15:22:00Z

Client/src/Components/ImageUpload/index.jsx

+    return /^[A-Za-z0-9+/=]+$/.test(data);
+  };
+
+const ImageUpload = ({ open, onClose, onUpdate, value, currentImage = value, theme, shouldRender = true, placeholder, maxSize, acceptedTypes, previewSize = 150, onError,}) => {


Theme doesn't need to be a prop, you can use the useTheme hook.

This is also an exceptionally complicated API 😂 Can we look at simplifying this? I don't think we need to take value and currentImage props as that can be maintained internally? The simpler the API the better.

Also feel free to create component variants as well to keep the API simple.

const ImageUpload = () => { ...} export const ImageUploadRoundPreview = () => { // Do stuff needed here <ImageUpload round={true}/> } export default ImageUpload

Something along those lines. The more complexity we can hide the better.

Now using the useTheme hook directly.
Removed unnecessary props.

I'm still Exploring with the component variants. Are there specific use cases you had in mind?

ajhollid · 2025-02-11T15:22:18Z

Client/src/Components/ImageUpload/index.jsx

+import CloudUploadIcon from "@mui/icons-material/CloudUpload";
+import { checkImage } from "../../Utils/fileUtils";
+import "./index.css";
+


Please include JSdoc for the component, see other components for examples

Please leave comments unresolved so other reviewers can see what's been happening and changes made 👍

ajhollid · 2025-02-11T15:22:35Z

Client/src/Components/ImageUpload/index.jsx

+import {GenericDialog} from "../Dialog/genericDialog";
+import CloudUploadIcon from "@mui/icons-material/CloudUpload";
+import { checkImage } from "../../Utils/fileUtils";
+import "./index.css";


We shouldn't be using css for styling, this can be removed

ajhollid · 2025-02-11T15:25:37Z

Client/src/Components/ImageUpload/index.jsx

+
+  const [file, setFile] = useState();
+  const [progress, setProgress] = useState({ value: 0, isLoading: false });
+  const [errors, setErrors] = useState({});


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 setErrors prop and allow the caller to handle the errors. Open to discussion on this one.

Updated the component to accept setErrors as a prop.
Errors are no longer handled internally. Let me know if this aligns with your expectation.

ajhollid · 2025-02-11T15:27:24Z

Client/src/Components/ImageUpload/index.jsx

+                <Stack
+                    className="custom-file-text"
+                    alignItems="center"
+                    gap="4px"


Please use theme.spacing() for gaps

ajhollid · 2025-02-11T15:27:42Z

Client/src/Components/ImageUpload/index.jsx

+                        color={theme.palette.primary.contrastTextTertiary}
+                        sx={{ opacity: 0.6 }}
+                    >
+                        (maximum size: 3MB)


This should be configurable by prop

ajhollid · 2025-02-11T15:28:33Z

Client/src/Components/ImageUpload/index.jsx

+                        <CloudUploadIcon />
+                    </IconButton>
+                    <Typography component="h2" color={theme.palette.primary.contrastTextTertiary}>
+                        <Typography component="span" fontSize="inherit" color="info" fontWeight={500}>


fontWeight should not be hardcoded, please use the theme. @marcelluscaio is the most knowledgeable on the theme so please reach out

Not sure we should be using color info, will take a look into that

ajhollid · 2025-02-11T15:30:19Z

Client/src/Components/ImageUpload/index.jsx

+        onClose={onClose}
+        theme={theme}
+        title={"Upload Image"}
+        description={"Select an image to upload."}


Strings should not be hardcoded anymore as we are moving towards providing localization. Please see the il8n.js file for details on the localization system. Example of implementation in the Login page. Documentation here

Replaced hardcoded strings with i18n

ajhollid · 2025-02-11T15:31:15Z

Client/src/Components/ImageUpload/index.jsx

+  } else if (typeof currentImage !== "undefined" && isValidBase64Image(currentImage)) {
+    imageSrc = `data:image/png;base64,${currentImage}`;
+  } else if (typeof placeholder !== "undefined") {
+    imageSrc = placeholder;


This value is never used?

vishnusn77 · 2025-02-13T04:46:11Z

I have implemented most of the requested changes. Let me know if it needs further changes.
Haven't messed with the hardcoded theme values yet. So that remains. I'll talk with @marcelluscaio and get that fixed as well.

ajhollid

There's a bug in here somewhere, if I click on "Delete" on the profile page it is removed from the preview, but if I go to "Update" then the photo is still loaded into the image upload component.

Uploading Screencast from 2025-02-13 10-56-57.webm…

ajhollid · 2025-02-13T18:45:56Z

Client/src/Components/ImageUpload/index.jsx

+import CloudUploadIcon from "@mui/icons-material/CloudUpload";
+import { checkImage } from "../../Utils/fileUtils";
+import "./index.css";
+


Please leave comments unresolved so other reviewers can see what's been happening and changes made 👍

ajhollid · 2025-02-13T18:46:49Z

Client/src/Components/ImageUpload/index.jsx

+        if (setErrors) setErrors?.((prev) => ({ ...prev, picture: errorMsg }));
+        return;
+    }
+


Cool, I like the early returns.

vishnusn77 · 2025-02-13T19:06:33Z

There's a bug in here somewhere, if I click on "Delete" on the profile page it is removed from the preview, but if I go to "Update" then the photo is still loaded into the image upload component.

Uploading Screencast from 2025-02-13 10-56-57.webm…

I'll look into that

marcelluscaio · 2025-02-13T20:09:49Z

Client/src/Components/ImageUpload/index.jsx

+  };
+
+  // Handles image file selection
+  const handlePicture = (event) => {


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

marcelluscaio · 2025-02-13T20:13:31Z

Client/src/Components/ImageUpload/index.jsx

+            return prevErrors;
+        });
+    }
+    return !!error;


What are the possibilities for the error returned?

marcelluscaio · 2025-02-13T20:20:42Z

Client/src/Components/TabPanels/Account/ProfilePanel.jsx

 							color="accent"
-							onClick={openPictureModal}
+							onClick={() => setIsOpen("picture")}


I prefer the approach where we pass down an abstraction of the setter function instead of the function itself. That way if implementation changes we dont need to touch the consumer, only the function that is being passed down

marcelluscaio · 2025-02-13T20:21:08Z

Client/src/Components/TabPanels/Account/ProfilePanel.jsx

+				onClose={() => setIsOpen("")}
 				onCancel={() => setIsOpen("")}


Same here, we shoudl avoid passing down the state setter

marcelluscaio · 2025-02-13T20:22:41Z

Client/src/Components/TabPanels/Account/ProfilePanel.jsx

+				setErrors={setErrors}
+				errors={errors}
+				open={isOpen === "picture"}
+				onClose={setIsOpen}


I would pass down a function that closes the modal, instead of passing the setter

marcelluscaio

It looks good in general, I made some suggestions, let me know you opinion on them

ajhollid · 2025-02-14T15:31:58Z

@marcelluscaio @vishnusn77 any ideas on simplifying this components API? The long list of props is making me nervous 😂

I realize this one is tricky as we're trying to refactor an existing component.

In your opinion would it make more sense to start from scratch and design a more reasonable API and then make the rest of the app conform to it? Or should we continue on and make the component conform to the application instead?

My instinct is to design an ideal component and the rest of the app to work with it 🤔

vishnusn77 · 2025-02-14T20:39:15Z

@marcelluscaio @vishnusn77 any ideas on simplifying this components API? The long list of props is making me nervous 😂

I realize this one is tricky as we're trying to refactor an existing component.

In your opinion would it make more sense to start from scratch and design a more reasonable API and then make the rest of the app conform to it? Or should we continue on and make the component conform to the application instead?

My instinct is to design an ideal component and the rest of the app to work with it 🤔

How about we wrap all the optional parameters into a single prop? Instead of passing multiple individual props like placeholder, maxSize, and acceptedTypes, we can do something like:

marcelluscaio · 2025-02-17T23:04:54Z

Client/src/Components/ImageUpload/index.jsx

+        <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",
+            }}
+        />


Is it possible to use MUIs Input component?

marcelluscaio · 2025-02-17T23:05:28Z

Client/src/Components/ImageUpload/index.jsx

+                    <Typography
+                        component="p"
+                        color={theme.palette.primary.contrastTextTertiary}
+                        sx={{ opacity: 0.6 }}


Have we tested if this passes contrast tests?

I think we can guide devs on how to do contrast tests - maybe a link or similar?

Absolutely!
Normally I use these two tools:

https://colourcontrast.cc/

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

marcelluscaio · 2025-02-17T23:12:30Z

Not sure about the config prop @vishnusn77 . It will create another lever for the props, but virtually the same props would be there.
I wonder if we need open and close on the parent level. What if button and modal were in the same component?

vishnusn77 added 8 commits February 10, 2025 16:35

create ImageUpload component and extract logic from profilepanel

2f80a9a

update profilepanel to use the imageupload component

2670375

pass theme prop to GenericDialog in ImageUpload component

edd98bf

resolve prop validation errors in GenericDialog and ImageField

223cad0

add logic to handle base64 images and placeholders

41733d2

update the rendering logic to use the new props

e9afb90

replace and comment image component usage for testing

8c3c23e

remove box material from imageupload component

81de3b4

vishnusn77 requested a review from ajhollid February 11, 2025 00:28

vishnusn77 added 12 commits February 10, 2025 21:32

revert back image component and usage

29d142c

add drag and drop event handlers to imageupload

d5ecf87

replace the usage of imagefield

b2237e6

display upload icon and message in drag drop field

c8b1fb7

replace input with textfield material

05854f5

add validation for files

c3f3353

fix preview image display issue

419d982

remove imagefield component and replace the usages with imageupload

b3599c1

fix image reupload issue by triggerring state change

dfcfee4

set drag and drop event handlers correctly

5b97b19

add prop to handle validation error

eedfdd3

added support for props to standardize API

808cc92

ajhollid requested changes Feb 11, 2025

View reviewed changes

vishnusn77 added 8 commits February 12, 2025 19:31

remove index.css file

3917b8b

remove internal error handling from imageupload component

1368ffa

merge drag and drop handling into single function

43be0a7

optimise imageupload component API

d4d78e8

refactor max size to configure by prop

28c4215

replace hardcoded strings with i18next

7657cd5

add jsdoc for imageupload component

ea18f50

error handling prop bug fix

a4f54f7

vishnusn77 requested a review from ajhollid February 13, 2025 04:49

ajhollid requested changes Feb 13, 2025

View reviewed changes

marcelluscaio reviewed Feb 13, 2025

View reviewed changes

marcelluscaio reviewed Feb 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/fe/image upload component #1737

Refactor/fe/image upload component #1737

vishnusn77 commented Feb 10, 2025 •

edited

Loading

ajhollid left a comment

ajhollid Feb 11, 2025 •

edited

Loading

vishnusn77 Feb 13, 2025

ajhollid Feb 11, 2025

ajhollid Feb 13, 2025

ajhollid Feb 11, 2025

ajhollid Feb 11, 2025

vishnusn77 Feb 13, 2025

ajhollid Feb 11, 2025

ajhollid Feb 11, 2025

vishnusn77 Feb 13, 2025

ajhollid Feb 11, 2025

marcelluscaio Feb 13, 2025

ajhollid Feb 11, 2025

vishnusn77 Feb 13, 2025

ajhollid Feb 11, 2025

vishnusn77 commented Feb 13, 2025

ajhollid left a comment •

edited

Loading

ajhollid Feb 13, 2025

ajhollid Feb 13, 2025

vishnusn77 commented Feb 13, 2025

marcelluscaio Feb 13, 2025

marcelluscaio Feb 13, 2025

marcelluscaio Feb 13, 2025

marcelluscaio Feb 13, 2025

marcelluscaio Feb 13, 2025

marcelluscaio left a comment

ajhollid commented Feb 14, 2025 •

edited

Loading

vishnusn77 commented Feb 14, 2025

marcelluscaio Feb 17, 2025

marcelluscaio Feb 17, 2025

gorkem-bwl Feb 17, 2025 •

edited

Loading

marcelluscaio Feb 17, 2025

marcelluscaio commented Feb 17, 2025

Refactor/fe/image upload component #1737

Are you sure you want to change the base?

Refactor/fe/image upload component #1737

Conversation

vishnusn77 commented Feb 10, 2025 • edited Loading

Describe your changes

Issue number

Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.

ajhollid left a comment

Choose a reason for hiding this comment

ajhollid Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishnusn77 commented Feb 13, 2025

ajhollid left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishnusn77 commented Feb 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelluscaio left a comment

Choose a reason for hiding this comment

ajhollid commented Feb 14, 2025 • edited Loading

vishnusn77 commented Feb 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gorkem-bwl Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelluscaio commented Feb 17, 2025

vishnusn77 commented Feb 10, 2025 •

edited

Loading

ajhollid Feb 11, 2025 •

edited

Loading

ajhollid left a comment •

edited

Loading

ajhollid commented Feb 14, 2025 •

edited

Loading

gorkem-bwl Feb 17, 2025 •

edited

Loading