Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

refactor: replace and remove deprecated Avatar component#15930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
BrunoQuaresma merged 32 commits intomainfrombq/use-new-avatar
Dec 20, 2024

Conversation

BrunoQuaresma
Copy link
Collaborator

Close#14997

@BrunoQuaresma
Copy link
CollaboratorAuthor

BrunoQuaresma commentedDec 18, 2024•
edited
Loading

During the refactor, I noticed that ShadCN adds some boilerplate to the Avatar component. Since we already have a well-established usage of this component across the codebase, I’m wondering if we could simplify it to the following:

Instead of:

<Avatar><Avatarsrc="..."/><AvatarFallback>{avatarLetter("Bruno Quaresma")}</AvatarFallback></Avatar>

It could be:

<Avatarsrc="..."alt="Bruno Quaresma"/>

@jaaydenh wdyt?

@jaaydenh
Copy link
Contributor

jaaydenh commentedDec 18, 2024•
edited
Loading

@BrunoQuaresma Are you proposing alt gets converted internally to the AvatarFallback? or not using the AvatarFallback? It feels like Avatar should have 3 props, src, alt and fallback. I definitely can see a benefit to simplifying the usage from 4 lines to 1 since Avatar is used in so many places.

BrunoQuaresma reacted with thumbs up emoji

@BrunoQuaresma
Copy link
CollaboratorAuthor

@BrunoQuaresma Are you proposing alt gets converted internally to the AvatarFallback? or not using the AvatarFallback? It feels like Avatar should have 3 props, src, alt and fallback. I definitely can see a benefit to simplifying the usage from 4 lines to 1 since Avatar is used in so many places.

Yes, I was initially thinking of usingalt to generate thefallback, but after some consideration, I agree with you—it should include the three props you mentioned instead. I’ll put up a PR with these changes.

@BrunoQuaresma
Copy link
CollaboratorAuthor

@jaaydenh it is ready for review! 👍

/>
{fallback && (
<AvatarPrimitive.Fallback className="flex h-full w-full items-center justify-center rounded-full">
{fallback.charAt(0).toUpperCase()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This could be a separate PR but it would be nice if we started using 2 letters for the fallback case. For example, the first letter of the first 2 words in display name. For example, if you look at the users page on dev.coder.com there are many duplicated avatars for the letters A and B that it doesn't serve much purpose as a identifier.

BrunoQuaresma reacted with thumbs up emoji
Copy link
CollaboratorAuthor

@BrunoQuaresmaBrunoQuaresmaDec 19, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, I was planning to address this in another PR if necessary. Since we don’t control the name or display name, we can’t enforce them to have at least two words to display two letters. In such cases, we would display only one letter, correct? Since this involves some logic and could slightly impact the design, I left it out of the scope for this PR.

@@ -35,7 +35,6 @@ export const UserDropdown: FC<UserDropdownProps> = ({
<div css={styles.badgeContainer}>
<Badge overlap="circular">
<UserAvatar
css={styles.avatar}
username={user.username}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

based on the designs for the topbar navigation,https://www.figma.com/design/WfqIgsTFXN2BscBSSyXWF8/Coder-kit?node-id=656-2354&t=ZPGincsj3IkIlTun-1

The size for the UserAvatar should besize="lg"

BrunoQuaresma reacted with thumbs up emoji
/>
);
export const TopbarAvatar: FC<{ src: string }> = ({ src }) => {
return <Avatar variant="icon" size="sm" src={src} />;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

the TopbarAvatar needs thefallback prop

BrunoQuaresma reacted with thumbs up emoji
default: "h-6 w-6 text-2xs",
sm: "h-[18px] w-[18px] text-[8px]",
lg: "h-[--avatar-lg] w-[--avatar-lg] rounded-[6px] text-sm font-medium",
default: "h-[--avatar-default] w-[--avatar-default] text-2xs",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Just to confirm, do you think default is being used alot more than lg and sm? otherwise I would name this "md"

BrunoQuaresma reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

For example, I used "default" and "sm" for Button.tsx because I imagined default would be used most of the time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

As I think about this more, I feel default makes alot more sense for a variant than it does for a size

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Makes sense. I think we can usemd instead.

@jaaydenh
Copy link
Contributor

@BrunoQuaresma one general comment is that I noticed you are not currently using the alt prop on the Avatar component.

BrunoQuaresma reacted with thumbs up emoji

@jaaydenh
Copy link
Contributor

UserAvatar.tsx andGroupAvatar.tsx do not seem to provide any extra value except handle the props slightly differently. Is there a specific use case for these? It feels like both could be removed without missing much.

BrunoQuaresma reacted with thumbs up emoji

@BrunoQuaresma
Copy link
CollaboratorAuthor

@BrunoQuaresma one general comment is that I noticed you are not currently using the alt prop on the Avatar component.

Good point. I think we can just remove it for now since theAvatar is more of a presentational component. What do you think?

@BrunoQuaresma
Copy link
CollaboratorAuthor

UserAvatar.tsx andGroupAvatar.tsx do not seem to provide any extra value except handle the props slightly differently. Is there a specific use case for these? It feels like both could be removed without missing much.

I’ll take a look, but that makes sense.

@jaaydenh
Copy link
Contributor

@BrunoQuaresma one general comment is that I noticed you are not currently using the alt prop on the Avatar component.

Good point. I think we can just remove it for now since theAvatar is more of a presentational component. What do you think?

I think though the alt prop gets passed to the underlying radix avatar component and is used as the alt prop for the image if the image doesn't load for some reason

@BrunoQuaresma
Copy link
CollaboratorAuthor

@BrunoQuaresma one general comment is that I noticed you are not currently using the alt prop on the Avatar component.

Good point. I think we can just remove it for now since theAvatar is more of a presentational component. What do you think?

I think though the alt prop gets passed to the underlying radix avatar component and is used as the alt prop for the image if the image doesn't load for some reason

Yep, but honestly, in this case, alt doesn’t add much value since we already use the avatar alongside other text or inside a button with a label (or aria-label). Since the avatar is more of a presentational element, I wouldn’t worry too much about it. That said, let’s involve@Parkreiner to see if it makes sense from a usability perspective.

@BrunoQuaresma
Copy link
CollaboratorAuthor

BrunoQuaresma commentedDec 20, 2024•
edited
Loading

@jaaydenh I think this PR is ready to merge. Regardingthis comment, I think we can circle back to it later if needed. What do you think?

Copy link
Contributor

@jaaydenhjaaydenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good now, I think it would be helpful to create an issue and tag christine to discuss the implementation for a 2 letter avatar fallback.

BrunoQuaresma reacted with thumbs up emoji
@BrunoQuaresma
Copy link
CollaboratorAuthor

@jaaydenh issue createdhere ✅

@BrunoQuaresmaBrunoQuaresma merged commit300ad87 intomainDec 20, 2024
29 checks passed
@BrunoQuaresmaBrunoQuaresma deleted the bq/use-new-avatar branchDecember 20, 2024 12:57
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsDec 20, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@jaaydenhjaaydenhjaaydenh approved these changes

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve default avatar style
2 participants
@BrunoQuaresma@jaaydenh
[8]ページ先頭
©2009-2025 Movatter.jp