Details

Reviewers

ashoat
atul

Commits

rCOMMb5df71ba24a0: [lib] introduce getAvatarForUser in avatar-utils

Summary

Introduce getAvatarForUser in avatar-utils. getAvatarForUser is a function that checks if a user has an avatar, and either returns that or randomly generates an emoji avatar for the user and returns that. We generate an emoji avatar based on the user's username. If the user does not have a username set, we just default to an avatar with this emoji 👤 for the user.

This diff won't be landed until there is a feature flag for the avatars work

Linear Task: https://linear.app/comm/issue/ENG-3257/create-function-to-automatically-select-an-emoji-avatar-for-users-with

Test Plan

I rendered the avatar (coming later in the stack) and checked to make sure the default emoji avatars didn't change for the same user

Ginsu's friends list

Screenshot 2023-03-13 at 1.22.55 AM.png (396×754 px, 70 KB)

Dan's avatar

Screenshot 2023-03-13 at 1.43.52 AM.png (1×1 px, 401 KB)

Ryan's avatar

Screenshot 2023-03-13 at 1.44.06 AM.png (1×1 px, 405 KB)

Unknown user:

Screenshot 2023-03-14 at 11.06.36 AM.png (1×1 px, 382 KB)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Mar 12 2023, 10:31 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMar 12 2023, 10:31 PM

ginsu edited the summary of this revision. (Show Details)Mar 12 2023, 10:46 PM

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

ginsu added reviewers: ashoat, atul.

ginsu attached a referenced file: F424841: Screenshot 2023-03-13 at 1.44.06 AM.png. (Show Details)

Harbormaster completed remote builds in B17270: Diff 23658.Mar 12 2023, 10:46 PM

ginsu requested review of this revision.Mar 12 2023, 10:46 PM

ginsu attached a referenced file: F424840: Screenshot 2023-03-13 at 1.43.52 AM.png. (Show Details)Mar 12 2023, 10:46 PM

ginsu attached a referenced file: F424838: Screenshot 2023-03-13 at 1.22.55 AM.png. (Show Details)

ginsu added inline comments.

lib/shared/avatar-utils.js
33 ↗	(On Diff #23658)	We will update this type in a subsequent diff when the logic for avatars with images are introduced

ginsu added a child revision: D7054: [keyserver/lib] fetch avatar column from users table in fetchCurrentUserInfo.Mar 12 2023, 10:55 PM

ashoat requested changes to this revision.Mar 13 2023, 9:29 AM

ashoat added inline comments.

lib/shared/avatar-utils.js
39 ↗	(On Diff #23658)	Is there a better emoji we can use for unknown users? Should we consider just omitting the avatar, and allowing the component that renders the avatar to handle displaying a default?
42 ↗	(On Diff #23658)	What's the benefit of making selection "stable" as opposed to selecting an emoji at random? We'll need a random selector anyways to allow the user to "reroll" a proposed emoji avatar, so I figured we might as well implement that from the start?

This revision now requires changes to proceed.Mar 13 2023, 9:29 AM

ashoat added inline comments.Mar 13 2023, 9:33 AM

lib/types/avatar-types.js
3 ↗	(On Diff #23658)	We should add a `type` here immediately. Otherwise we'll have to deal with clients that have an avatar without a type, and default those to the "emoji" type

ashoat added inline comments.Mar 13 2023, 9:38 AM

lib/shared/avatar-utils.js
42 ↗	(On Diff #23658)	Ah okay, this is used on the client side for determining an emoji for a given user that doesn't have one set, which is why it needs to be "stable" (This wasn't clear until I read D7057)

ginsu added inline comments.Mar 13 2023, 2:06 PM

lib/shared/avatar-utils.js
39 ↗	(On Diff #23658)	Yea definitely open to changing the emoji. I went with 😀 because I felt it was the safest option. Please lmk if you had another emoji or something else in mind Should we consider just omitting the avatar, and allowing the component that renders the avatar to handle displaying a default? I'll further explore this idea. Initially, I considered implementing it while building but decided against it. I anticipated reusing the avatar component when introducing avatars for threads and thought it would be better to first create the avatar info logic outside the component and then pass in the avatar info into the avatar component (as seen in D7057) instead of passing in either user or thread info into the avatar component and then creating the avatar info inside of the comopnent. Lmk if this approach is okay, if we feel better about allowing the avatar component to determine the default avatar. I can make the changes.
42 ↗	(On Diff #23658)	We'll need a random selector anyways to allow the user to "reroll" a proposed emoji avatar, so I figured we might as well implement that from the start? I can introduce this in a follow up diff when we introduce the "reroll" logic

ashoat added inline comments.Mar 13 2023, 2:42 PM

lib/shared/avatar-utils.js
39 ↗	(On Diff #23658)	Yea definitely open to changing the emoji. I went with 😀 because I felt it was the safest option. Please lmk if you had another emoji or something else in mind Can you try Googling for "anonymous emoji" to try to find some alternatives? Yea definitely open to changing the emoji. I went with 😀 because I felt it was the safest option. Please lmk if you had another emoji or something else in mind Should we consider just omitting the avatar, and allowing the component that renders the avatar to handle displaying a default? I'll further explore this idea. Initially, I considered implementing it while building but decided against it. I anticipated reusing the avatar component when introducing avatars for threads and thought it would be better to first create the avatar info logic outside the component and then pass in the avatar info into the avatar component (as seen in D7057) instead of passing in either user or thread info into the avatar component and then creating the avatar info inside of the comopnent. Lmk if this approach is okay, if we feel better about allowing the avatar component to determine the default avatar. I can make the changes. When I asked that question earlier, I wasn't clear on where this function was supposed to be used. Now that I realize it's called from the render component, it's clear that you're basically already doing what I suggested: "allowing the component that renders the avatar to handle displaying a default".

ginsu added inline comments.Mar 13 2023, 3:10 PM

lib/shared/avatar-utils.js
39 ↗	(On Diff #23658)	Can you try Googling for "anonymous emoji" to try to find some alternatives? Getting a ton of results that have the anonymous hacker group mask on an emoji lol. Other things I found are the classic no profile picture human outline: In the figma I did find this design we could use as the "anonymous emoji" (We would get rid the upload photo floating button) To be honest, not a huge fan of any of these options. I will think more about this later tonight and ask Ted if he has any ideas to improve this
lib/types/avatar-types.js
3 ↗	(On Diff #23658)	Just to clarify this comment you are referring to a field called `type`, or are you are referring to creating more types for example an `ImageAvatar`, and an `AvatarInfo` type which could be `EmojiAvatar \| ImageAvatar`