Details

Reviewers

ashoat
atul

Commits

rCOMM112846eaebdd: [lib] introduce useGetAvatarForThread hook

Summary

introduce useGetAvatarForThread hook. When thinking about thread avatars, there are three main thread avatars we need to consider

channels/group chats
private aka the chat with yourself (introduced in D7178)
personal aka 1on1 chat with another user (introduced in D7178)

Another thing we needed to consider when building this hook is that the thread avatar should default the channel thread avatar to their community's avatar, but we do need to should special-case GENESIS and make it an exception (otherwise, all DMs / group chats will default to the GENESIS avatar)

Depends on D7178

Test Plan

Please check out these screenshots to see that the thread avatars are getting defaulted to the community avatar for non-genesis communities, and all chats under genesis are getting a randomly generated thread avatar

Non-genesis community:

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

Genesis chats:

Screenshot 2023-03-24 at 3.04.22 PM.png (1×1 px, 830 KB)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Mar 21 2023, 10:38 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMar 21 2023, 10:38 PM

Harbormaster completed remote builds in B17495: Diff 23963.Mar 21 2023, 10:54 PM

ginsu requested review of this revision.Mar 21 2023, 10:54 PM

Discovered bug with the avatar generating new random avatar when creating a new chat or sub channel will update this when resolved

ginsu added a child revision: D7133: [lib] introduce getAvatarForThreadEntity function.Mar 22 2023, 12:02 AM

Mostly questions

lib/shared/avatar-utils.js
66–73 ↗	(On Diff #23963)	Why aren't we just using `ThreadInfo` or `RawThreadInfo` or `ThreadInfo \| RawThreadInfo` here?
85 ↗	(On Diff #23963)	We will need to call this hook regardless of whether we want to use a thread avatar, so it makes sense that the input is optional. However, I'm not sure it makes sense to return `defaultAnonymousUserEmojiAvatar`... I think it makes sense for the calling component to handle the "default" behavior (Or is this meant as the general-purpose hook for getting an avatar for a thread? If so, why is the input nullable?)
92 ↗	(On Diff #23963)	I wonder if we should also default channels to their community's avatar. If so, we could replace this check with a `thread.containingThreadID` check. For sidebars, `containingThreadID` is the parent. For other chats, it's the containing community.
107 ↗	(On Diff #23963)	Curious if we'll need to handle ENS avatar resolution in two places: here (for your `PRIVATE` thread if you're an ENS user) and the calling component (for `PERSONAL` 1:1 thread, if the other person is an ENS user). Ideally we can avoid having to call the ENS resolution hook twice. This should be possible to avoid if we handle `PERSONAL` here.
110 ↗	(On Diff #23963)	Does `PERSONAL` get handled here or elsewhere?

ginsu mentioned this in D7178: [lib] introduce getUserAvatarForThread function.Mar 25 2023, 11:16 PM

new version of useGetAvatarForThread

change order

ginsu edited the summary of this revision. (Show Details)Mar 25 2023, 11:34 PM

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

ginsu added a parent revision: D7178: [lib] introduce getUserAvatarForThread function.

ginsu attached a referenced file: F445489: Screenshot 2023-03-24 at 3.04.13 PM.png. (Show Details)

ginsu attached a referenced file: F445490: Screenshot 2023-03-24 at 3.04.22 PM.png. (Show Details)

ginsu removed a child revision: D7133: [lib] introduce getAvatarForThreadEntity function.

ginsu removed a parent revision: D7131: [lib] introduce getDefaultAvatar helper function.

Harbormaster completed remote builds in B17637: Diff 24139.Mar 25 2023, 11:36 PM

remove stray ?

Harbormaster completed remote builds in B17638: Diff 24140.Mar 25 2023, 11:39 PM

In D7132#212051, @ashoat wrote:

Mostly questions

I'm hoping that between D7178 and the updates in this diff, all the questions you asked should be answered; however, if there is a question you feel didn't get properly addressed, please let me know, and I can provide further elaboration.

Harbormaster completed remote builds in B17639: Diff 24141.Mar 25 2023, 11:52 PM

ginsu added a child revision: D7134: [native] update message list header to new design.Mar 26 2023, 12:02 AM

ashoat accepted this revision.Mar 26 2023, 5:53 AM

This revision is now accepted and ready to land.Mar 26 2023, 5:53 AM

rebase before landing

Harbormaster completed remote builds in B17649: Diff 24153.Mar 26 2023, 7:05 PM

ginsu mentioned this in rCOMM82a9fdc14701: [lib] introduce getUserAvatarForThread function.Mar 26 2023, 7:13 PM

Closed by commit rCOMM112846eaebdd: [lib] introduce useGetAvatarForThread hook (authored by ginsu). · Explain WhyMar 26 2023, 7:16 PM

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMM112846eaebdd: [lib] introduce useGetAvatarForThread hook.