Page MenuHomePhabricator

[native] introude thread avatar component
ClosedPublic

Authored by ginsu on Mar 30 2023, 3:03 PM.
Tags
None
Referenced Files
F3516092: D7262.diff
Sun, Dec 22, 12:10 PM
Unknown Object (File)
Tue, Dec 17, 7:24 PM
Unknown Object (File)
Sun, Dec 15, 4:05 AM
Unknown Object (File)
Mon, Dec 9, 5:24 AM
Unknown Object (File)
Sat, Dec 7, 7:22 AM
Unknown Object (File)
Fri, Dec 6, 12:27 AM
Unknown Object (File)
Thu, Dec 5, 8:38 PM
Unknown Object (File)
Wed, Dec 4, 3:33 AM
Subscribers

Details

Summary

After some discussion with @ashoat and @atul about how to fetch the ens avatars we decided to have two separate components to render avatars: UserAvatar and ThreadAvatar. This diff handles ThreadAvatar.

Generally, a thread avatar should not be able an ENS avatar since we won't be able to set it for threads; however, if the thread type is either PRIVATE or PERSONAL then if the user avatar type is ens then we need to render the ens user avatar

Depends on D7260

Test Plan

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

Emoji:

Screenshot 2023-03-30 at 5.11.49 PM.png (1×1 px, 939 KB)

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

Screenshot 2023-03-30 at 5.07.25 PM.png (1×1 px, 947 KB)

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

Screenshot 2023-03-30 at 5.06.21 PM.png (1×1 px, 932 KB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu requested review of this revision.Mar 30 2023, 3:19 PM
ashoat requested changes to this revision.Mar 30 2023, 5:34 PM

Looks good but see comments in D7260. I think we should move the userInfo fetch here and pass in ensAddress to useAvatarForUserID (which should get renamed to eg. useENSResolvedAvatar)

native/components/thread-avatar.react.js
40

Irrelevant to this stack, but looking at this now I think it would be more clear to name this useAvatarForThread. Especially now that we have useAvatarForUserID

Could you rename it in a separate diff?

This revision now requires changes to proceed.Mar 30 2023, 5:35 PM
native/components/thread-avatar.react.js
19

Also see my comment on D7263 – I think we should pass RawThreadInfo | ThreadInfo directly in here, since it looks like all of the callsites have that already. No need fetching it again if we already have it

address ashoat's comments

native/components/thread-avatar.react.js
40

One concern inline, please address before landing

native/components/thread-avatar.react.js
41–43 ↗(On Diff #24453)

This should be in the useSelector, I think. Otherwise any change to userInfos will cause this component to rerender, even if the change did not affect the relevant user

The tradeoffs when deciding what part to include in useSelector, versus what part to do later are:

  1. Performance of useSelector: useSelector blocks get executed whenever ANY Redux state changes, so it's good for them to be quick / performant
  2. Rerendering the component: if the useSelector block returns a new result, it causes the whole component to rerender, so it's good to only select the part of the data you care about
This revision is now accepted and ready to land.Mar 31 2023, 5:34 AM

address ashoat's comments/rebase before landing

This revision was automatically updated to reflect the committed changes.