Details

Reviewers

ashoat
atul

Commits

rCOMM7676f85478bb: [web] introduce avatar component

Summary

Introduced Avatar for web. Right now the component only handles emoji and image avatars but in a follow up diff when I can better test ENS avatars I will extend the component to support ens as well

I didn't include the avatars feature flag for the web avatars rendering work because as of now the feature flag service tomek built is only available on native, and I figured since we are close to having avatars released and this avatar component considers both emoji and images, building an extra feature flag service for the web avatars would have not been a good use of time. However, if we disagree with my sentiment, I can add the feature flag to web as well. With all this said though, I won't land the rendering work in subsequent diffs until avatars is good to go

Test Plan

Please see the screenshots to see the avatars rendered on the web client:

Emoji Avatars:

micro, small, and large:

Screenshot 2023-03-27 at 9.49.46 AM.png (2×4 px, 1 MB)

profile:

Screenshot 2023-03-27 at 9.49.51 AM.png (2×4 px, 933 KB)

Image Avatars:

micro, small, and large:

Screenshot 2023-03-27 at 9.50.26 AM.png (2×4 px, 1 MB)

profile:

Screenshot 2023-03-27 at 9.50.29 AM.png (2×4 px, 968 KB)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Mar 27 2023, 6:47 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMar 27 2023, 6:48 AM

ginsu edited the summary of this revision. (Show Details)Mar 27 2023, 6:59 AM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: ashoat, atul.

ginsu attached a referenced file: F447176: Screenshot 2023-03-27 at 9.50.29 AM.png. (Show Details)

ginsu attached a referenced file: F447175: Screenshot 2023-03-27 at 9.50.26 AM.png. (Show Details)

ginsu attached a referenced file: F447174: Screenshot 2023-03-27 at 9.49.51 AM.png. (Show Details)

ginsu attached a referenced file: F447164: Screenshot 2023-03-27 at 9.49.46 AM.png. (Show Details)

Harbormaster completed remote builds in B17668: Diff 24188.Mar 27 2023, 7:03 AM

ginsu requested review of this revision.Mar 27 2023, 7:03 AM

ginsu added a child revision: D7194: [web] render user avatar in account screen.Mar 27 2023, 7:04 AM

atul accepted this revision.Mar 27 2023, 10:35 AM

atul added inline comments.

web/components/avatar.css
7–17 ↗	(On Diff #24188)	I'd group these and put them in some order so this is more readable. EG .emojiMicro {...} .emojiSmall {...} .emojiLarge {...}
web/components/avatar.react.js
10–13 ↗	(On Diff #24188)	Can we put these in order from smallest to largest?
18–23 ↗	(On Diff #24188)	Would be helpful if these were ordered throughout.

This revision is now accepted and ready to land.Mar 27 2023, 10:35 AM

address atuls comments

Harbormaster completed remote builds in B17701: Diff 24228.Mar 27 2023, 11:50 AM

ashoat added inline comments.Mar 27 2023, 4:26 PM

web/components/avatar.react.js
64 ↗	(On Diff #24228)	Why aren't we just returning `avatar` directly? Pretty sure this is a no-op, and it ruins the memoization you set up in `avatar` above You generally only want to use `React.Fragment` to wrap an array of things, or to assign a `key` to some `React.Node` you were passed Also note that `<>` is a good shorthand for `React.Fragment`. It only doesn't work if you're assigning a `key` (Ignore if this is used later in the stack or something)

ginsu added inline comments.Mar 28 2023, 12:31 PM

web/components/avatar.react.js
64 ↗	(On Diff #24228)	You generally only want to use React.Fragment to wrap an array of things, or to assign a key to some React.Node you were passed Didn't know about this, but that makes sense. thanks for tip!