-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
ref(scraps) simplify avatar component #107799
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
885832b
142877c
b4042a4
6982ee2
db1ef82
7c0c0f1
6a70cdc
f5b8069
a9b86ed
f9accea
215d7b3
2ed8f98
f8f3b8a
e8096d3
671763d
0d3b4d7
77115bc
2bf5c8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,7 +93,11 @@ function CommitRow({ | |
| <Message>{formatCommitMessage(commit.message)}</Message> | ||
| )} | ||
| <MetaWrapper> | ||
| {customAvatar ? customAvatar : <UserAvatar size={16} user={commit.author} />} | ||
| {customAvatar ? ( | ||
| customAvatar | ||
| ) : commit.author ? ( | ||
| <UserAvatar size={16} user={commit.author} /> | ||
| ) : null} | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. null rendering was previously implicit when user was falsy |
||
| <Meta hasStreamlinedUI> | ||
| <Tooltip | ||
| title={tct( | ||
|
|
@@ -193,16 +197,14 @@ function CommitRow({ | |
| </EmailWarning> | ||
| } | ||
| > | ||
| <UserAvatar size={36} user={commit.author} /> | ||
| {commit.author ? <UserAvatar size={36} user={commit.author} /> : null} | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. null rendering was previously implicit when user was falsy |
||
| <EmailWarningIcon data-test-id="email-warning"> | ||
| <IconWarning size="xs" /> | ||
| </EmailWarningIcon> | ||
| </Hovercard> | ||
| </AvatarWrapper> | ||
| ) : ( | ||
| <div> | ||
| <UserAvatar size={36} user={commit.author} /> | ||
| </div> | ||
| <div>{commit.author ? <UserAvatar size={36} user={commit.author} /> : null}</div> | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. null rendering was previously implicit when user was falsy |
||
| )} | ||
|
|
||
| <CommitMessage> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| import type React from 'react'; | ||
| import {useMemo} from 'react'; | ||
| import * as Sentry from '@sentry/react'; | ||
|
|
||
|
|
@@ -7,7 +6,7 @@ import type {Actor} from 'sentry/types/core'; | |
| import {useMembers} from 'sentry/utils/useMembers'; | ||
| import {useTeamsById} from 'sentry/utils/useTeamsById'; | ||
|
|
||
| import {BaseAvatar, type BaseAvatarProps} from './baseAvatar'; | ||
| import {Avatar, type AvatarProps} from './avatar'; | ||
| import {TeamAvatar, type TeamAvatarProps} from './teamAvatar'; | ||
| import {UserAvatar, type UserAvatarProps} from './userAvatar'; | ||
|
|
||
|
|
@@ -16,13 +15,11 @@ interface SimpleActor extends Omit<Actor, 'name'> { | |
| name?: string; | ||
| } | ||
|
|
||
| export interface ActorAvatarProps extends BaseAvatarProps { | ||
| export interface ActorAvatarProps extends Omit<AvatarProps, 'round'> { | ||
| actor: SimpleActor; | ||
| ref?: React.Ref<HTMLSpanElement | SVGSVGElement | HTMLImageElement>; | ||
| } | ||
|
|
||
| export function ActorAvatar({ | ||
| ref, | ||
| size = 24, | ||
| hasTooltip = true, | ||
| actor, | ||
|
|
@@ -35,11 +32,11 @@ export function ActorAvatar({ | |
| }; | ||
|
|
||
| if (actor.type === 'user') { | ||
| return <AsyncMemberAvatar userActor={actor} {...otherProps} ref={ref} />; | ||
| return <AsyncMemberAvatar actor={actor} {...otherProps} />; | ||
| } | ||
|
|
||
| if (actor.type === 'team') { | ||
| return <AsyncTeamAvatar teamId={actor.id} {...otherProps} ref={ref} />; | ||
| return <AsyncTeamAvatar teamId={actor.id} {...otherProps} />; | ||
| } | ||
|
|
||
| Sentry.withScope(scope => { | ||
|
|
@@ -56,33 +53,34 @@ export function ActorAvatar({ | |
|
|
||
| interface AsyncTeamAvatarProps extends Omit<TeamAvatarProps, 'team'> { | ||
| teamId: string; | ||
| ref?: React.Ref<HTMLSpanElement | SVGSVGElement | HTMLImageElement>; | ||
| } | ||
|
|
||
| function AsyncTeamAvatar({ref, teamId, ...props}: AsyncTeamAvatarProps) { | ||
| function AsyncTeamAvatar({teamId, ...props}: AsyncTeamAvatarProps) { | ||
| const {teams, isLoading} = useTeamsById({ids: [teamId]}); | ||
| const team = teams.find(t => t.id === teamId); | ||
|
|
||
| if (isLoading) { | ||
| const size = `${props.size}px`; | ||
| return <Placeholder width={size} height={size} />; | ||
| return <Placeholder width={`${props.size}px`} height={`${props.size}px`} />; | ||
| } | ||
|
|
||
| if (!team) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This used to be implicit |
||
| return <Avatar type="letter_avatar" name={teamId} identifier={teamId} {...props} />; | ||
| } | ||
|
|
||
| return <TeamAvatar team={team} {...props} ref={ref} />; | ||
| return <TeamAvatar team={team} {...props} />; | ||
| } | ||
|
|
||
| /** | ||
| * Wrapper to assist loading the user from api or store | ||
| */ | ||
| interface AsyncMemberAvatarProps extends Omit<UserAvatarProps, 'user'> { | ||
| userActor: SimpleActor; | ||
| ref?: React.Ref<HTMLSpanElement | SVGSVGElement | HTMLImageElement>; | ||
| interface AsyncMemberAvatarProps extends Omit<UserAvatarProps, 'user' | 'round'> { | ||
| actor: SimpleActor; | ||
| } | ||
|
|
||
| function AsyncMemberAvatar({ref, userActor, ...props}: AsyncMemberAvatarProps) { | ||
| const ids = useMemo(() => [userActor.id], [userActor.id]); | ||
| function AsyncMemberAvatar({actor, ...props}: AsyncMemberAvatarProps) { | ||
| const ids = useMemo(() => [actor.id], [actor.id]); | ||
| const {members, fetching} = useMembers({ids}); | ||
| const member = members.find(u => u.id === userActor.id); | ||
| const member = members.find(u => u.id === actor.id); | ||
|
|
||
| if (fetching) { | ||
| const size = `${props.size}px`; | ||
|
|
@@ -91,14 +89,15 @@ function AsyncMemberAvatar({ref, userActor, ...props}: AsyncMemberAvatarProps) { | |
|
|
||
| if (!member) { | ||
| return ( | ||
| <BaseAvatar | ||
| ref={ref} | ||
| size={props.size} | ||
| title={userActor.name ?? userActor.email} | ||
| <Avatar | ||
| {...props} | ||
| type="letter_avatar" | ||
| name={actor.name ?? actor.email ?? actor.id} | ||
| identifier={actor.id} | ||
| round | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| return <UserAvatar user={member} {...props} ref={ref} />; | ||
| return <UserAvatar user={member} {...props} />; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| import {render, screen} from 'sentry-test/reactTestingLibrary'; | ||
|
|
||
| // eslint-disable-next-line boundaries/entry-point | ||
| import {Avatar} from './avatar'; | ||
|
|
||
| describe('Avatar', () => { | ||
| describe('upload URL size parameter', () => { | ||
| it('appends ?s=120 to upload URLs', () => { | ||
| render( | ||
| <Avatar | ||
| type="upload" | ||
| uploadUrl="https://example.com/avatar.jpg" | ||
| identifier="test-id" | ||
| name="Test User" | ||
| /> | ||
| ); | ||
| const img = screen.getByRole('img'); | ||
| expect(img).toHaveAttribute('src', 'https://example.com/avatar.jpg?s=120'); | ||
| }); | ||
|
|
||
| it('appends &s=120 to upload URLs with existing query params', () => { | ||
| render( | ||
| <Avatar | ||
| type="upload" | ||
| uploadUrl="https://example.com/avatar.jpg?version=2" | ||
| identifier="test-id" | ||
| name="Test User" | ||
| /> | ||
| ); | ||
| const img = screen.getByRole('img'); | ||
| expect(img).toHaveAttribute( | ||
| 'src', | ||
| 'https://example.com/avatar.jpg?version=2&s=120' | ||
| ); | ||
| }); | ||
|
|
||
| it('does not modify data URLs', () => { | ||
| const dataUrl = | ||
| 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+M9QDwADhgGAWjR9awAAAABJRU5ErkJggg=='; | ||
| render( | ||
| <Avatar type="upload" uploadUrl={dataUrl} identifier="test-id" name="Test User" /> | ||
| ); | ||
| const img = screen.getByRole('img'); | ||
| // Data URLs should not have size parameter appended | ||
| expect(img).toHaveAttribute('src', dataUrl); | ||
| }); | ||
| }); | ||
|
|
||
| describe('avatar type rendering', () => { | ||
| it('renders ImageAvatar for upload type', () => { | ||
| render( | ||
| <Avatar | ||
| type="upload" | ||
| uploadUrl="https://example.com/avatar.jpg" | ||
| identifier="test-id" | ||
| name="Test User" | ||
| /> | ||
| ); | ||
| expect(screen.getByRole('img')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders LetterAvatar for letter_avatar type', () => { | ||
| render(<Avatar type="letter_avatar" identifier="test-id" name="Test User" />); | ||
| expect(screen.getByText('TU')).toBeInTheDocument(); | ||
| expect(screen.queryByRole('img')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders Gravatar for gravatar type', async () => { | ||
| render( | ||
| <Avatar | ||
| type="gravatar" | ||
| gravatarId="test@example.com" | ||
| identifier="test-id" | ||
| name="Test User" | ||
| /> | ||
| ); | ||
| // Gravatar hashes asynchronously, so use findBy | ||
| expect(await screen.findByRole('img')).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('tooltip', () => { | ||
| it('shows tooltip when hasTooltip=true', () => { | ||
| render( | ||
| <Avatar | ||
| type="letter_avatar" | ||
| identifier="test-id" | ||
| name="Test User" | ||
| tooltip="Test Tooltip" | ||
| hasTooltip | ||
| /> | ||
| ); | ||
| expect(screen.getByText('TU')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('does not show tooltip when hasTooltip=false', () => { | ||
| render( | ||
| <Avatar | ||
| type="letter_avatar" | ||
| identifier="test-id" | ||
| name="Test User" | ||
| tooltip="Test Tooltip" | ||
| hasTooltip={false} | ||
| /> | ||
| ); | ||
| expect(screen.getByText('TU')).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UX here is unnecessary and confusing. We already indicate in the option that users can control their gravatar at gravatar.com, and there is no reason to display a upload and a change button that do the same thing