Page MenuHomePhabricator

[lib] introduce useGetAvatarForThread hook
ClosedPublic

Authored by ginsu on Mar 21 2023, 10:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jul 7, 12:45 AM
Unknown Object (File)
Sat, Jul 6, 8:30 AM
Unknown Object (File)
Thu, Jul 4, 3:21 AM
Unknown Object (File)
Wed, Jul 3, 3:49 AM
Unknown Object (File)
Tue, Jul 2, 8:55 PM
Unknown Object (File)
Tue, Jul 2, 8:03 AM
Unknown Object (File)
Tue, Jul 2, 1:33 AM
Unknown Object (File)
Sun, Jun 30, 10:33 PM
Subscribers

Details

Summary

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

  1. channels/group chats
  2. private aka the chat with yourself (introduced in D7178)
  3. 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 edited the summary of this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: ashoat, atul.
ginsu edited the test plan for this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)

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

ashoat requested changes to this revision.Mar 22 2023, 12:12 PM

Mostly questions

lib/shared/avatar-utils.js
66–73

Why aren't we just using ThreadInfo or RawThreadInfo or ThreadInfo | RawThreadInfo here?

85

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

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

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

Does PERSONAL get handled here or elsewhere?

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

new version of useGetAvatarForThread

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.

This revision is now accepted and ready to land.Mar 26 2023, 5:53 AM
This revision was automatically updated to reflect the committed changes.