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.

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

Only use image thumbnail instead of full sized profile image #1568

Merged
merged 9 commits into from
Apr 2, 2021

Conversation

thienlnam
Copy link
Contributor

@thienlnam thienlnam commented Feb 26, 2021

<If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit.>

Details

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/146391

Tests

On web:

  • Right click a profile picture on the chats list, hit 'Inspect' make sure the image elements ends with .128.jpg like this one <img alt="" draggable="false" src="https://d2g02b6ed2w9fz.cloudfront.net/0c2101577e36c53d943549c696610aa142668700_**128.jpeg**" class="css-accessibilityImage-9pa8cd">

  • Do the same with a profile picture in an actual chat

Other devices: Make sure profile images show up and look as expected in the sidebar and in the chat

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-03-25 at 8 14 39 PM
Screen Shot 2021-03-25 at 8 14 44 PM

cc @tgolen @nickmurray47 since you guys are reviewing the Web-E PR for this

@thienlnam thienlnam self-assigned this Feb 26, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Feb 26, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@thienlnam
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@thienlnam thienlnam changed the title [HOLD] Only use image thumbnail instead of full sized profile image [HOLD 30362] Only use image thumbnail instead of full sized profile image Mar 26, 2021
@thienlnam thienlnam marked this pull request as ready for review March 26, 2021 03:45
@thienlnam thienlnam requested a review from a team as a code owner March 26, 2021 03:45
@botify botify requested review from Beamanator and removed request for a team March 26, 2021 03:45
@thienlnam
Copy link
Contributor Author

Holding on PR but can still be reviewed / tested by pulling down the Web-E PR

@shawnborton
Copy link
Contributor

Just curious, but what is the actual size of the icon we're serving up now? For images to appear high quality and retina, we want them to be at least 2x or 3x the size of the area in which they are displayed. So for example, avatars are 40x40 in the LHN, ideally we serve up an image that is 120x120. The only other area to consider is the profile view when you tap on your own icon and the Settings page launches. I believe we show the avatar at 80x80, so we'd want the image in that case to come through as at least 240x240. Let me know if that all makes sense!

@thienlnam
Copy link
Contributor Author

In the LHN right now, the size of the images we're serving for the icons are actually the full-sized profile images that were uploaded as a profile picture and varies from person to person. For example, it looks like the image served for your profile picture is 1365x1365 in the LHN, and the ones I'm seeing at a glance are between like 334x334 to 3024x3026. This change should make it so that all the images are loaded as 128x128 for the 40x40 icon sizes.

Also I see what you're saying about the 80x80, I'll go ahead and see if we have the option to serve images at that resolution and update that one location

@shawnborton
Copy link
Contributor

Sounds great, thanks!

@Beamanator
Copy link
Contributor

I'm having a hard time seeing the new, smaller images in the LHN - The code lodashGet(details, [dmParticipant, 'avatarThumbnail'], '') is always undefined for me, so the LHN thumbnails are never loading - is there something else I need to do to get that to populate?

@thienlnam
Copy link
Contributor Author

Do you have master Web-E pulled down? The Web-E changes required for this are currently deployed on staging but it adds the 'avatarThumbnail' parameter that you're currently seeing as undefined

@Beamanator Beamanator self-requested a review April 1, 2021 09:42
Beamanator
Beamanator previously approved these changes Apr 1, 2021
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Tested once the Web-E changes got to Production - everything looks great! :D

LGTM - I assume this isn't on Hold anymore since the Web-E PR is merged & in production, right?

@thienlnam
Copy link
Contributor Author

@shawnborton It doesn't look like we can just grab a 240x240 version of an image like we can with the 128 versions, since the source image is still larger than 80x80, can this be put into another issue so we can just push this out? The alternative is just loading the full resolution image for all 80x80 locations right now which wouldn't be the most performant

@thienlnam
Copy link
Contributor Author

thienlnam commented Apr 1, 2021

@Beamanator Yup! Looks like the PR is now in production so this should be safe to go out pending Shawns review. I did add back the personalDetails check in the if statement in my latest push

@thienlnam thienlnam changed the title [HOLD 30362] Only use image thumbnail instead of full sized profile image Only use image thumbnail instead of full sized profile image Apr 1, 2021
@thienlnam
Copy link
Contributor Author

Looks like Shawn is OOO for the rest of the week, since this is attached to a daily could you give it another look and merge if it looks good? @Beamanator

Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Looks great! I tested on Desktop and looks good too :D

@Beamanator Beamanator merged commit c052e58 into master Apr 2, 2021
@Beamanator Beamanator deleted the jack-updateavatar branch April 2, 2021 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants