Page MenuHomePhabricator

[lib] introduce getAvatarForUser in avatar-utils
ClosedPublic

Authored by ginsu on Mar 12 2023, 10:31 PM.
Tags
None
Referenced Files
F1434028: D7053.id23935.diff
Thu, Mar 28, 8:18 AM
F1434024: D7053.id23927.diff
Thu, Mar 28, 8:18 AM
F1434023: D7053.id23895.diff
Thu, Mar 28, 8:18 AM
F1434022: D7053.id23715.diff
Thu, Mar 28, 8:18 AM
F1434021: D7053.id23848.diff
Thu, Mar 28, 8:18 AM
F1434020: D7053.id23658.diff
Thu, Mar 28, 8:18 AM
F1434004: D7053.id.diff
Thu, Mar 28, 8:17 AM
F1433997: D7053.diff
Thu, Mar 28, 8:13 AM

Details

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 edited the test plan for this revision. (Show Details)
ginsu added reviewers: ashoat, atul.
ginsu added inline comments.
lib/shared/avatar-utils.js
33

We will update this type in a subsequent diff when the logic for avatars with images are introduced

ashoat requested changes to this revision.Mar 13 2023, 9:29 AM
ashoat added inline comments.
lib/shared/avatar-utils.js
39

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

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
lib/types/avatar-types.js
3

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

lib/shared/avatar-utils.js
42

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)

lib/shared/avatar-utils.js
39

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

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

lib/shared/avatar-utils.js
39

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".

lib/shared/avatar-utils.js
39

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:

Screenshot 2023-03-13 at 5.55.15 PM.png (912×1 px, 263 KB)

Screenshot 2023-03-13 at 5.55.07 PM.png (838×888 px, 205 KB)

In the figma I did find this design we could use as the "anonymous emoji" (We would get rid the upload photo floating button)

Screenshot 2023-03-13 at 6.02.57 PM.png (1×1 px, 106 KB)

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

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

lib/shared/avatar-utils.js
39

Those are images, I meant emojis

lib/types/avatar-types.js
3

The latter, add +type: 'emoji' here

varun added inline comments.
lib/shared/avatar-utils.js
39
lib/shared/avatar-utils.js
39

That looks perfect!

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

address feedback

ginsu edited the test plan for this revision. (Show Details)
This revision is now accepted and ready to land.Mar 14 2023, 12:25 PM

use new avatar types introduced by @atul