In amazon s3 #55

g4rry420 · 2021-06-10T19:18:47Z

What this PR does (required):

Added the integration of profile photos to backend
IN: Amazon S3 #26

Screenshots / Videos (required):

Any information needed to test this feature (required):

Go to /settings url and navigate to Profile Photo tab to test this feature.

Any issues with the current functionality (optional):

No

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

bonnieli

looks good, just some readability and rewriting some repeated code comments.

in addition looks like you're disabling a lot of eslint warnings - what is currently the warning?

bonnieli · 2021-06-14T17:56:34Z

client/src/components/ProfilePhoto/ProfilePhoto.tsx

-      setImgSrc(src);
+  const { updateSnackBarMessage } = useSnackBar();
+  const handleImageUpload = (event: ChangeEvent<HTMLInputElement>): void => {
+    if (userState.background === '' && event.target.files?.length) {


maybe store event.target.files in a variable - you're accessing it multiple times in this block

Sure, will do that.

bonnieli · 2021-06-14T17:57:00Z

client/src/components/ProfilePhoto/ProfilePhoto.tsx

+    if (userState.background === '' && event.target.files?.length) {
+      const background = URL.createObjectURL(event.target.files[0]);
+      dispatchUserContext({ type: 'UPLOAD_BACKGROUND', background });
+      // eslint-disable-next-line


what is currently the eslint issue here?

Forbidden non-null assertion @typescript-eslint/no-non-null-assertion

This is resolved when I store event.target.files in a variable.

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

moffatethan · 2021-06-15T20:36:56Z

client/src/pages/Settings/Settings.tsx

+  // eslint-disable-next-line
+  const handleTabIndexChange = (event: any, newValue: number): void => {


I am going to assume you are disabling the 'no-implicit-any' warning here. I would suggest to add a type to that event and remove the eslint warning that way.

The reason I didn't specify type of this is because this is backlog issue with material-ui. However, I guess I can specify this type ChangeEvent<{}> but then this type gives me these errors:

Line 46:52: Don't use `{}` as a type. `{}` actually means "any non-nullish value". - If you want a type meaning "any object", you probably want `Record<string, unknown>` instead. - If you want a type meaning "any value", you probably want `unknown` instead. - If you want a type meaning "empty object", you probably want `Record<string, never>` instead @typescript-eslint/ban-types

I would take a look at where you are firing the event and it generally gives you an idea of what the type should look like. Event types are tricky in React, I tend to just start at the source of where I am firing this event and then copy what I see in my editor.

Have you seen the MUI contributors comment here?

Actually, I have tried the SyntheticEvent but it doesn't work. But following your advise, I looked at the console.log of event and found out that the event type is BaseSyntheticEvent. Thank you for your help.

moffatethan · 2021-06-15T21:04:20Z

server/controllers/s3.js

+        // store locationUrl in Database;
+        const locationUrl = data.Location;
+        locationUrls.push({ locationUrl, key: data.key })
+        if (locationUrls.length === fieldNames.length) {
+          return res.status(201).json({
+            locationUrls,
+            success: "File are saved successfully."
+          })


I see a comment about adding it to the database but there is no persisting the location urls to the profile model. Also, what if a user wants to just upload a profile photo? Right now on the front-end it requires them to upload a background image then a profile image. For a UX perspective this is not ideal.

Yes, I have handle the usecase for location urls in #57 pr.
Also, I understand that this is not a very good UX experience. Initially, I wanted to do something like this. But material ui doesn't support this right now. What I can do is provide a radio button to user to choose which image they want to upload. It's not very good UX, but it will decent one. What are your thoughts on this ?

It's something that can be addressed later, I would do some further research into potential solution here. I will say, Material UI is not blocking this feature from happening. But, feel free to visit this later.

Sure, okay.

moffatethan

It's looking good, I left some comments on your PR to address.

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

g4rry420 · 2021-06-16T00:57:51Z

@moffatethan I have made the changes. Please let me know about them.

moffatethan · 2021-06-16T21:28:53Z

client/src/pages/Dashboard/Dashboard.tsx

@@ -30,6 +30,7 @@ export default function Dashboard(): JSX.Element {
  return (
    <Grid container component="main" className={`${classes.root} ${classes.dashboard}`}>
      <CssBaseline />
+      <Link to="/settings">Profile</Link>


Couldn't this be in the AuthNavbar?

Sure, will do

moffatethan

It looks good to me, before merging if you could take a look into the placement of the settings link and move it to a more suitable file such as AuthNavbar. Thank you!

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

g4rry420 added 5 commits June 8, 2021 03:02

Added background and profile image in Profile Photo page

d9a4d96

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

Added frontend upload integration to backend

617e17b

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

remove the expires from the params in s3 controller

8953d36

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

Added the interface and userContext for images

cd1ed58

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

removed unecessary comments

d428c28

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

g4rry420 linked an issue Jun 10, 2021 that may be closed by this pull request

IN: Amazon S3 #26

Closed

g4rry420 added 2 commits June 10, 2021 15:22

Resolved conflicts

eaa8423

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

resolved conflicts

ca70fd7

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

g4rry420 mentioned this pull request Jun 14, 2021

IN_Profile_Page #57

Merged

bonnieli reviewed Jun 14, 2021

View reviewed changes

Refactored as suggested by bonnie

56dbe8c

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

moffatethan reviewed Jun 15, 2021

View reviewed changes

moffatethan suggested changes Jun 15, 2021

View reviewed changes

Changed the UI for selecting image in profile photo for better UX

2095124

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

g4rry420 requested a review from moffatethan June 16, 2021 20:12

moffatethan reviewed Jun 16, 2021

View reviewed changes

moffatethan approved these changes Jun 16, 2021

View reviewed changes

removed link from dashboard component

8227466

Signed-off-by: gurkiran_singh <gurkiransinghk@gmail.com>

g4rry420 merged commit f56f6a6 into main Jun 16, 2021

g4rry420 deleted the IN_amazon_s3 branch June 16, 2021 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In amazon s3 #55

In amazon s3 #55

g4rry420 commented Jun 10, 2021 •

edited

Loading

bonnieli left a comment

bonnieli Jun 14, 2021

g4rry420 Jun 14, 2021

bonnieli Jun 14, 2021

g4rry420 Jun 14, 2021

g4rry420 Jun 14, 2021

moffatethan Jun 15, 2021

g4rry420 Jun 15, 2021

moffatethan Jun 15, 2021

moffatethan Jun 15, 2021

g4rry420 Jun 16, 2021

moffatethan Jun 15, 2021

g4rry420 Jun 16, 2021

moffatethan Jun 16, 2021

g4rry420 Jun 16, 2021

moffatethan left a comment

g4rry420 commented Jun 16, 2021

moffatethan Jun 16, 2021

g4rry420 Jun 16, 2021

moffatethan left a comment

		// eslint-disable-next-line
		const handleTabIndexChange = (event: any, newValue: number): void => {

In amazon s3 #55

In amazon s3 #55

Conversation

g4rry420 commented Jun 10, 2021 • edited Loading

What this PR does (required):

Screenshots / Videos (required):

Any information needed to test this feature (required):

Any issues with the current functionality (optional):

bonnieli left a 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

Choose a reason for hiding this comment

moffatethan left a comment

Choose a reason for hiding this comment

g4rry420 commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moffatethan left a comment

Choose a reason for hiding this comment

g4rry420 commented Jun 10, 2021 •

edited

Loading