Page MenuHomePhabricator

[native] introduce user avatar component
ClosedPublic

Authored by ginsu on Mar 29 2023, 4:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 8, 1:38 AM
Unknown Object (File)
Tue, Mar 5, 11:17 PM
Unknown Object (File)
Tue, Mar 5, 11:17 PM
Unknown Object (File)
Tue, Mar 5, 11:17 PM
Unknown Object (File)
Feb 25 2024, 10:07 AM
Unknown Object (File)
Feb 12 2024, 3:57 AM
Unknown Object (File)
Feb 12 2024, 3:49 AM
Unknown Object (File)
Feb 12 2024, 3:48 AM
Subscribers

Details

Summary

After some discussion with @ashoat and @atul about how to fetch the ens uri for ens avatars we decided to have two separate components to render avatars: UserAvatar and ThreadAvatar this diff handles UserAvatar and a future diff will handle ThreadAvatar.

Here is a quick overview of why we went this way (thanks @atul):

Forgive me if I'm missing something, but this is what I was thinking for user avatars.

  1. userStore.userInfos is the "source of truth" for ClientAvatars on the client.
  2. The ClientAvatar for a given user can be directly queried from userStore.userInfos with a userID.
  3. Therefore, the Avatar component ONLY needs a userID prop in order to get "everything it needs" to render the user avatar.
  4. The simplest possible API for the Avatar component is a single userID prop.
  5. The Avatar component can use userID and a hook named eg useClientAvatarForUserID in order to query userStore.userInfos to get the ClientAvatar for a given user (if it exists)

This prevents us from having to compute anything "outside" of the Avatar component to then be passed in. All we need is to pass in the userID, which we almost certainly have anywhere we're displaying Avatars since they're usually displayed alongside username. I also think it's cleaner and easier to reasonable about if everything happens internally "within" the Avatar component instead of counting on the containing component to determine what should be displayed in the contained Avatar component.

Depends on D7187

Test Plan

I was able to render all three avatar types: Emoji, Image, and ENS

Emoji:

Screenshot 2023-03-29 at 7.26.58 PM.png (1×1 px, 808 KB)

Image by hardcoding: const avatarInfo = { type: 'image', uri: 'https://picsum.photos/200' }

Screenshot 2023-03-29 at 7.19.51 PM.png (1×1 px, 811 KB)

ENS by hardcoding const avatarInfo = { type: 'ens' } and the other values like the provider and ethAddress from this gist

Screenshot 2023-03-29 at 7.18.58 PM.png (1×952 px, 588 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/components/user-avatar.react.js
55 ↗(On Diff #24354)

A future diff will handle making the avatarInfo prop type in Avatar better and more strict. It was just difficult to do it in this diff since we render Avatar from other places atm and making the type more strict was making flow unhappy.

ginsu requested review of this revision.Mar 29 2023, 4:42 PM
native/components/user-avatar.react.js
28–36 ↗(On Diff #24354)

FYI: this was difficult for me to test and verify that we are getting the correct ethAddress since I don't have my own eth address or Ethereum account. However, this logic was taken from here which I assume is correct.

Could be good if someone who has a working eth address or Ethereum account to double check this

ashoat added inline comments.
native/components/user-avatar.react.js
28–36 ↗(On Diff #24354)

Can you set up an ETH account? It's fairly trivial, just install the Rainbow wallet on your phone. You don't need to spend any money

55 ↗(On Diff #24354)

Are you sure we should be passing avatarInfo to Avatar? I think earlier I suggested just passing a uri in... any reason that wouldn't work?

This revision is now accepted and ready to land.Mar 30 2023, 6:14 AM
native/components/user-avatar.react.js
55 ↗(On Diff #24354)

We still want to consider emoji avatars, so just passing a uri wouldn't work. So my thinking is that this avatarInfo prop type is going to either be :

{
  +type: 'emoji',
  +emoji: string,
  +color: string, // hex, without "#" or "0x"
} |
{
  +type: 'image',
  +uploadID: string,
};

(will type this more elegantly when I actually push the diff up, but just wanted to make sure this is super clear)

Where emoji avatars will be the first type and image and ens avatars will be the second type after going through UserAvatar. Let me know if that doesn't make sense or if I am misunderstanding something

native/components/user-avatar.react.js
28–36 ↗(On Diff #24354)

Just tried it, and when trying to create my ENS profile it does cost ~$30. Spoke to @atul and he also said that there is a Comm.eth profile I could also use. Still happy to make my own ens profile if that's helpful, but just wanted to make sure we were aware of the cost

native/components/user-avatar.react.js
55 ↗(On Diff #24354)

please note the avatar info type above for images should be

{
  +type: 'image',
  +uri: string,
};

not

{
  +type: 'image',
  +uploadID: string,
};
This revision was automatically updated to reflect the committed changes.