Changed Profile Picture save on click of Save button #7366

mananjadhav · 2022-01-22T07:23:58Z

Details

For ProfilePage we were saving it immediately after uploading, which was a bit inconsistent from Workspace. To have a uniform user experience, with this PR, now it'll update the profile picture on click of the Save button.

Fixed Issues

$ #7015

Tests

Uploading a new profile photo
Save button being disabled if the picture is not updated
Removing the existing profile picture
Reloading the screens without clicking Save (or going back)

Verify that no errors appear in the JS console

QA Steps

Go to the settings and click on Avatar
In the sidebar click on the profile avatar
Click on the edit (or camera) icon of Avatar and upload the photo
Ensure it doesn't upload the photo
Click on Save and verify that the avatar is saved

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

web-profile-picture-update.mov

Mobile Web

mweb-profile-image-update_cAd3mnGv.mp4

Desktop

desktop-profile-image-update_RIANWcN4.mp4

iOS

ios-profile-image-update.mp4

Android

android-profile-image-update_c9u6RQul.mp4

mananjadhav · 2022-01-22T10:12:55Z

@rushatgabhane @AndrewGable I have got one question. From the UX standpoint, if you see the toast message shows while the image is being uploaded at the background. Because we stay at the same page I don't think so it needs any handling.

But do let me know if you want to move setAvatar and deleteAvatar inside PersonalDetails.setPersonalDetails call.

rushatgabhane · 2022-01-23T03:56:20Z

Proposal hasn't been given a green yet, so I'll wait until that's done before PR review.

mananjadhav · 2022-01-23T04:43:47Z

Ohh sorry for the confusion, I took this assignment comment as for hire #7015 (comment)

do you want me to close the PR ?

rushatgabhane · 2022-01-23T07:53:23Z

That's okay. Umm, convert it to a draft I guess

rushatgabhane

Overall looks good, minor change suggested.

I'm fine with the growl shown while the image is uploading. Because in case of a large image/slow network, it'll be confusing to see the success message long after exiting ProfilePage.

src/pages/settings/Profile/ProfilePage.js

rushatgabhane · 2022-01-24T12:56:59Z

@mananjadhav The save button isn't disabled after uploading new avatar.

1.New.Expensify.-.Google.Chrome.2022-01-24.15-55-54.mp4

mananjadhav · 2022-01-25T18:12:21Z

@rushatgabhane I tried updating the state of avatarURL based on the props change, but there's a small flicker. Are we fine with that?

Screen.Recording.2022-01-25.at.11.06.22.PM.mov

rushatgabhane · 2022-01-25T18:48:00Z

Hmm, that's not ideal. We should fix the flicker.

rushatgabhane · 2022-01-27T19:09:35Z

@mananjadhav please merge main into this branch

mananjadhav · 2022-01-28T14:02:53Z

@rushatgabhane Sorry for the delay, been unwell. I will resolve the conflict when working on the flicker change.

I have thought about the flicker but I have a few questions.

It should disable only if the image is successfully uploaded right? With this if I consider then on componentDidUpdate, the URL changes from a local imageUri to remote URL of the avatar, with this change, it will flicker. In Workspace the thing is the image is uploaded, the URL is returned and that is what gets set hence we don't see the flicker.

Now either I can use a flag/condition, which probably updates on profile image upload complete, say avatarImg !== null && this.props.isAvatarUpload === false,

or I store a state variable like isAvatarUpdateComplete, which gets set if the if (this.props.myPersonalDetails.avatar !== this.state.avatarPreviewURL && this.props.myPersonalDetails.avatar !== prevProps.myPersonalDetails.avatar) {

and use that to disable the button.

Thoughts?

rushatgabhane · 2022-01-28T22:47:50Z

Hi @mananjadhav, that's okay. I hope you're well now :)

It should disable only if the image is successfully uploaded right?

I don't think that's a good idea. If the button isn't disabled, the user might press it again.
Image upload takes time and we shouldn't resend those requests.

rushatgabhane · 2022-01-29T04:22:58Z

I'll get back to you regarding flicker.
Meanwhile, let me know if you're able to figure something out.

…ofile-image-call # Conflicts: # src/pages/settings/Profile/ProfilePage.js

mananjadhav · 2022-01-30T07:23:23Z

I don't think that's a good idea. If the button isn't disabled, the user might press it again.

@rushatgabhane I guess I didn't elaborate on all cases. I've updated the code of what I was explaining and it seems to be working. Thoughts?

rushatgabhane · 2022-01-30T14:31:51Z

Thanks for the update @mananjadhav, the save button isn't disabled until upload is complete.

Screencast.from.30-01-22.05-24-20.PM.+03.mp4

I can't find a way around the flicker without waiting for the upload either.

Sorry to drag this, but let's revert and go to the previous implementation?
(Button disable immediately)

src/pages/settings/Profile/ProfilePage.js

rushatgabhane

@mananjadhav I found a bug.

Remove profile photo.
Click Save button.
Change timezone.

Expected result: Save button is enabled, and has styling of enabled button (100% opacity).

Actual result: Save button is enabled, but it has styling of disabled button (50% opacity).

Screen.Recording.2022-01-31.at.5.14.30.AM.mov

mananjadhav · 2022-01-31T06:03:59Z

@rushatgabhane I had specifically tested this piece and I am not able to reproduce it. Can you help with the reproduction steps?

Here's the video while I did the testing.

profile-image-update-test.mp4

rushatgabhane · 2022-01-31T06:32:51Z

@mananjadhav hmm, this is interesting. You're following the correct steps.

I retested and I'm facing this issue on all platforms :(
I'm able to replicate it on a different laptop too.

mananjadhav · 2022-01-31T06:39:39Z

I didn't check on a different device, but I tested it all on platforms. I did all my testing on Chrome earlier but I saw your screencast is in Safari, so I tested on Safari as well.

rushatgabhane · 2022-01-31T06:46:51Z

@mananjadhav thanks!

@AndrewGable could you please help us verify this? Because it isn't reproducible for Manan.

rushatgabhane · 2022-01-31T07:21:10Z

@mananjadhav This is weird, but for me the Growl message is shown twice. I know this isn't the case for you as seen in the video above.

I clicked only once, branch is on latest commit with nothing to commit, tested on all platforms and 2 laptops.

I think there's something wrong on my end that I can't figure out, let's see what @AndrewGable says :)
Again, thanks for your patience!

Screen.Recording.2022-01-31.at.9.37.36.AM.mov

mananjadhav · 2022-02-02T12:50:01Z

@AndrewGable Can you help me and @rushatgabhane with this one?

rushatgabhane · 2022-02-03T14:19:18Z

@mananjadhav I just tested it on a third device. I can consistently repro this bug and Growl message shown twice.

Let's revert to - #7366 (comment)
It'll keep the code easy to understand too. What do you think?

I'm on the latest commit

mananjadhav · 2022-02-03T15:23:52Z

Hmm, 🤔 Let me probably try this again.

rushatgabhane · 2022-02-07T11:36:19Z

Any updates? Thanks

…ofile-image-call

mananjadhav · 2022-02-07T19:02:52Z

@rushatgabhane Thanks for being persistent with this. I was able to reproduce this and just pushed a fix for this.

rushatgabhane

@AndrewGable LGTM! 🎉️ Tests well on all platforms and all bugs are resolved.

Thanks so much @mananjadhav for making the changes.

OSBotify · 2022-02-08T16:58:28Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-02-09T18:05:13Z

🚀 Deployed to staging by @AndrewGable in version: 1.1.38-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-02-15T18:50:55Z

🚀 Deployed to production by @Julesssss in version: 1.1.38-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

NikkiWines · 2022-02-16T19:32:09Z

Looks like this caused a regression @mananjadhav @AndrewGable - #7784

mananjadhav · 2022-02-16T19:45:41Z

On it

Beamanator · 2022-02-19T14:22:17Z

src/pages/settings/Profile/ProfilePage.js

        // Recalculate logins if loginList has changed
-        if (this.props.user.loginList === prevProps.user.loginList) {
+        if (this.props.user.loginList !== prevProps.user.loginList) {


Can this ever be false? I'm pretty sure in JS you can never check if arrays (objects) are equal with === or !== because they're pointers

mananjadhav added 3 commits January 22, 2022 12:22

fix: Removed growl

b8f1b3b

fix: Changed update image to store in the state

0cc1735

fix: Add API calls to set or delete avatar

ac92139

mananjadhav marked this pull request as ready for review January 22, 2022 10:07

mananjadhav requested a review from a team as a code owner January 22, 2022 10:07

botify requested review from AndrewGable and rushatgabhane January 22, 2022 10:07

MelvinBot removed the request for review from a team January 22, 2022 10:07

mananjadhav marked this pull request as draft January 23, 2022 08:21

AndrewGable mentioned this pull request Jan 23, 2022

[HOLD for payment 2022-03-08] Feature request - Change the profile picture (api call) on click of the Save button - reported by @mananjadhav #7015

Closed

rushatgabhane suggested changes Jan 24, 2022

View reviewed changes

src/pages/settings/Profile/ProfilePage.js Outdated Show resolved Hide resolved

Julesssss assigned mananjadhav Jan 25, 2022

mananjadhav marked this pull request as ready for review January 25, 2022 18:11

botify requested a review from rushatgabhane January 25, 2022 18:11

fix: code cleanup

89a5706

mananjadhav added 2 commits January 30, 2022 12:49

fix: Disable button when the avatar is updated

b5d1524

Merge branch 'main' of https://github.com/mananjadhav/App into fix/pr…

a52d233

…ofile-image-call # Conflicts: # src/pages/settings/Profile/ProfilePage.js

rushatgabhane reviewed Jan 30, 2022

View reviewed changes

src/pages/settings/Profile/ProfilePage.js Show resolved Hide resolved

mananjadhav added 2 commits January 30, 2022 22:00

fix: Added new line

0431bf2

fix: Added method bindings

8ac0bea

rushatgabhane suggested changes Jan 31, 2022

View reviewed changes

fix: Added isDefaultAvatar for "Remove Photo" option

66b0e06

mananjadhav added 2 commits February 7, 2022 23:49

Merge branch 'main' of https://github.com/mananjadhav/App into fix/pr…

88ee869

…ofile-image-call

feat: Removed growl on Avatar update

67a670e

rushatgabhane approved these changes Feb 8, 2022

View reviewed changes

AndrewGable approved these changes Feb 8, 2022

View reviewed changes

AndrewGable merged commit a517f53 into Expensify:main Feb 8, 2022

OSBotify mentioned this pull request Feb 9, 2022

Deploy Checklist: New Expensify 2022-02-09 #7655

Closed

49 tasks

mvtglobally mentioned this pull request Feb 16, 2022

Changing or Adding firstName or lastName in profile page removes the profile photo - reported by @sobitneupane #7784

Closed

mananjadhav mentioned this pull request Feb 16, 2022

fix: Fixed profile image removed if personal info changed #7785

Merged

7 tasks

NikkiWines mentioned this pull request Feb 17, 2022

Simplify profile avatar setting logic #7789

Merged

5 tasks

Beamanator reviewed Feb 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed Profile Picture save on click of Save button #7366

Changed Profile Picture save on click of Save button #7366

mananjadhav commented Jan 22, 2022 •

edited

Loading

mananjadhav commented Jan 22, 2022

rushatgabhane commented Jan 23, 2022

mananjadhav commented Jan 23, 2022

rushatgabhane commented Jan 23, 2022

rushatgabhane left a comment •

edited

Loading

rushatgabhane commented Jan 24, 2022

mananjadhav commented Jan 25, 2022

rushatgabhane commented Jan 25, 2022

rushatgabhane commented Jan 27, 2022

mananjadhav commented Jan 28, 2022 •

edited

Loading

rushatgabhane commented Jan 28, 2022 •

edited

Loading

rushatgabhane commented Jan 29, 2022 •

edited

Loading

mananjadhav commented Jan 30, 2022

rushatgabhane commented Jan 30, 2022

rushatgabhane left a comment

mananjadhav commented Jan 31, 2022

rushatgabhane commented Jan 31, 2022

mananjadhav commented Jan 31, 2022

rushatgabhane commented Jan 31, 2022 •

edited

Loading

rushatgabhane commented Jan 31, 2022 •

edited

Loading

mananjadhav commented Feb 2, 2022

rushatgabhane commented Feb 3, 2022

mananjadhav commented Feb 3, 2022

rushatgabhane commented Feb 7, 2022

mananjadhav commented Feb 7, 2022

rushatgabhane left a comment •

edited

Loading

OSBotify commented Feb 8, 2022

OSBotify commented Feb 9, 2022

OSBotify commented Feb 15, 2022

NikkiWines commented Feb 16, 2022

mananjadhav commented Feb 16, 2022

Beamanator Feb 19, 2022

Changed Profile Picture save on click of Save button #7366

Changed Profile Picture save on click of Save button #7366

Conversation

mananjadhav commented Jan 22, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

mananjadhav commented Jan 22, 2022

rushatgabhane commented Jan 23, 2022

mananjadhav commented Jan 23, 2022

rushatgabhane commented Jan 23, 2022

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

rushatgabhane commented Jan 24, 2022

mananjadhav commented Jan 25, 2022

rushatgabhane commented Jan 25, 2022

rushatgabhane commented Jan 27, 2022

mananjadhav commented Jan 28, 2022 • edited Loading

rushatgabhane commented Jan 28, 2022 • edited Loading

rushatgabhane commented Jan 29, 2022 • edited Loading

mananjadhav commented Jan 30, 2022

rushatgabhane commented Jan 30, 2022

rushatgabhane left a comment

Choose a reason for hiding this comment

mananjadhav commented Jan 31, 2022

rushatgabhane commented Jan 31, 2022

mananjadhav commented Jan 31, 2022

rushatgabhane commented Jan 31, 2022 • edited Loading

rushatgabhane commented Jan 31, 2022 • edited Loading

mananjadhav commented Feb 2, 2022

rushatgabhane commented Feb 3, 2022

mananjadhav commented Feb 3, 2022

rushatgabhane commented Feb 7, 2022

mananjadhav commented Feb 7, 2022

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

OSBotify commented Feb 8, 2022

OSBotify commented Feb 9, 2022

OSBotify commented Feb 15, 2022

NikkiWines commented Feb 16, 2022

mananjadhav commented Feb 16, 2022

Beamanator Feb 19, 2022

Choose a reason for hiding this comment

mananjadhav commented Jan 22, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

mananjadhav commented Jan 28, 2022 •

edited

Loading

rushatgabhane commented Jan 28, 2022 •

edited

Loading

rushatgabhane commented Jan 29, 2022 •

edited

Loading

rushatgabhane commented Jan 31, 2022 •

edited

Loading

rushatgabhane commented Jan 31, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading