ref(scraps) refactor and test avatar component#107799
ref(scraps) refactor and test avatar component#107799
Conversation
| customAvatar | ||
| ) : commit.author ? ( | ||
| <UserAvatar size={16} user={commit.author} /> | ||
| ) : null} |
There was a problem hiding this comment.
null rendering was previously implicit when user was falsy
| } | ||
| > | ||
| <UserAvatar size={36} user={commit.author} /> | ||
| {commit.author ? <UserAvatar size={36} user={commit.author} /> : null} |
There was a problem hiding this comment.
null rendering was previously implicit when user was falsy
| <div> | ||
| <UserAvatar size={36} user={commit.author} /> | ||
| </div> | ||
| <div>{commit.author ? <UserAvatar size={36} user={commit.author} /> : null}</div> |
There was a problem hiding this comment.
null rendering was previously implicit when user was falsy
| @@ -64,7 +64,7 @@ export function ReleaseCommit({commit}: ReleaseCommitProps) { | |||
| <CommitContent> | |||
| <Message>{formatCommitMessage(commit.message)}</Message> | |||
| <MetaWrapper> | |||
| <UserAvatar size={16} user={commit.author} /> | |||
| {commit.author ? <UserAvatar size={16} user={commit.author} /> : null} | |||
There was a problem hiding this comment.
null rendering was previously implicit when user was falsy
| @@ -99,7 +99,7 @@ function CommitAuthorBreakdown({orgId, projectSlug, version}: Props) { | |||
| > | |||
| {sortedAuthorsByNumberOfCommits.map(({commitCount, author}, index) => ( | |||
| <AuthorLine key={author?.email ?? index}> | |||
| <UserAvatar user={author} size={20} hasTooltip /> | |||
| {author ? <UserAvatar user={author} size={20} hasTooltip /> : null} | |||
There was a problem hiding this comment.
null rendering was previously implicit when user was falsy
| const Avatar = styled(ActorAvatar)<{index: number; reverse: boolean}>` | ||
| ${translateStyles} | ||
| transform: translateX(${p => (p.reverse ? 60 * p.index : 60 * -p.index)}%); |
| <UserAvatar | ||
| user={user} | ||
| size={parseInt(iconSize, 10)} | ||
| // gravatar={false} |
There was a problem hiding this comment.
Avatars are visual identifier, and we should aim for them to be as stable as possible. There should never be a case where a user avatar is displayed as a letter avatar on one page vs a gravatar on all the rest.
| <Flex align="center" gap="md" paddingLeft="xs"> | ||
| {hasAssigneeMatch && userForAvatar ? ( | ||
| <UserAvatar user={userForAvatar} size={24} gravatar /> | ||
| <UserAvatar user={userForAvatar} size={24} /> |
There was a problem hiding this comment.
Avatars are visual identifier, and we should aim for them to be as stable as possible. There should never be a case where a user avatar is displayed as a letter avatar on one page vs a gravatar on all the rest.
| return ( | ||
| <StyledPanelItem key={commit.id} data-test-id="quick-context-commit-row"> | ||
| <UserAvatar size={24} user={commit.author} /> | ||
| {commit.author ? <UserAvatar size={24} user={commit.author} /> : null} |
There was a problem hiding this comment.
null rendering was previously implicit when user was falsy
| letterId: user.name, | ||
| title: user.name, | ||
| uploadUrl: '', | ||
| function getUserAvatarProps( |
There was a problem hiding this comment.
It is now much clearer that these are just thin wrappers around our Avatar component. It also opens up the case where we can say that these are Sentry specific, not scraps, and should live somewhere in the sentry/components folder.
| @@ -1,28 +0,0 @@ | |||
| import {render, screen} from 'sentry-test/reactTestingLibrary'; | |||
There was a problem hiding this comment.
These files have been moved to sub folders, as they are the building block that our entity avatars build on
| @@ -1,97 +0,0 @@ | |||
| import {useEffect, useState} from 'react'; | |||
There was a problem hiding this comment.
These files have been moved to sub folders, as they are the building block that our entity avatars build on
| ); | ||
| } | ||
|
|
||
| function AvatarList({ |
There was a problem hiding this comment.
This was the last module that was using the default export
| export function Avatar({ | ||
| ref, | ||
| className, | ||
| size, | ||
| style, | ||
| tooltip, | ||
| tooltipOptions, | ||
| hasTooltip = false, | ||
| ...props | ||
| }: GravatarBaseAvatarProps | LetterBaseAvatarProps | UploadBaseAvatarProps) { | ||
| return ( | ||
| <Tooltip title={tooltip} disabled={!hasTooltip} {...tooltipOptions} skipWrapper> |
There was a problem hiding this comment.
Avatar component is the abstraction over Gravatar, Letter or ImageAvatar and the one that all entity avatars use
| return <Placeholder width={`${props.size}px`} height={`${props.size}px`} />; | ||
| } | ||
|
|
||
| if (!team) { |
0a4f454 to
331d80f
Compare
40eb67e to
f597fbd
Compare
| const gravatarActions = ( | ||
| <AvatarActions> | ||
| <LinkButton | ||
| external | ||
| href="https://gravatar.com" | ||
| size="zero" | ||
| priority="transparent" | ||
| icon={<IconOpen />} | ||
| aria-label={t('Go to gravatar.com')} | ||
| title={t('Visit gravatar.com to upload your Gravatar to be used on Sentry.')} | ||
| /> | ||
| </AvatarActions> | ||
| ); |
f597fbd to
e8a0c03
Compare
ref(baseAvatar) inline avatar address comments streamline hook call and reset ref(avatar) resolve error handling avatar: add new primitives avatar: add new primitives avatar: improve types ref(avatar) wip test(avatar): rewrite LetterAvatar tests to focus on public API Rewrote tests to focus solely on the component's public behavior: - Initials rendering for various name formats - Fallback to question mark when name is empty or whitespace - Proper handling of multibyte characters, emails, and edge cases Removed tests for internal implementation details like styling and refs. test(avatar): add ImageAvatar tests for public API Added comprehensive tests for ImageAvatar component covering: - Image rendering when src is valid - Fallback to LetterAvatar when src is missing or image fails to load - Proper handling of src changes and error state reset - Props passing to both image and fallback components All tests focus on observable behavior rather than implementation details. test(avatar): enhance Gravatar tests to cover public API Enhanced Gravatar tests to cover: - SHA-256 hashing of gravatarId and URL generation - Remote size and d=404 parameters in URL - Fallback to LetterAvatar for empty/whitespace gravatarId - Props passing to both gravatar image and fallback - Hash regeneration when gravatarId changes - Switching between valid and empty gravatarId All tests focus on observable behavior and public API. fix(avatar): fix ImageAvatar fallback not triggering Fixed issues that prevented the LetterAvatar fallback from displaying when image load fails: 1. Properly destructure `name` from props before spreading to prevent it from being passed to the Image component 2. Remove incorrect `alt` prop being passed from Avatar component to ImageAvatar (ImageAvatarProps explicitly omits 'alt') These changes ensure the onError handler from mergeProps is properly attached without prop conflicts, allowing the error state to trigger and display the LetterAvatar fallback when an image fails to load. fix avatar correct usage correct usage correct usage use fixtures revert entire html interface update avatar chooser
e8a0c03 to
142877c
Compare
Updated test expectations to match the new avatar behavior where initials are generated from display names instead of slugs. Changes: - Team avatar: Expected initials "TS" → "TN" (from "Team Name" not "team-slug") - Organization avatar: Expected initials "OS" → "ON" (from "Organization Name" not "org-slug") - Changed test ID from "default-avatar" → "letter_avatar-avatar" to match new Avatar component behavior This is the correct behavior - avatars should use human-readable names for initials rather than technical slugs.
The Avatar component was not respecting custom data-test-id props passed from parent components. This caused test failures in components that relied on custom test IDs (e.g., 'assigned-avatar' in assignedTo.tsx). Updated Avatar to accept and prefer custom data-test-id props while falling back to the generated type-based ID when no custom ID is provided.
| className, | ||
| size, | ||
| style, | ||
| tooltip, | ||
| tooltipOptions, | ||
| hasTooltip = false, |
There was a problem hiding this comment.
I will clean these up in a followup PR. I am thinking of moving it under tooltipProps and then codemoding it along with tooltipProps on our button component
| @@ -1,6 +1,6 @@ | |||
| import groupBy from 'lodash/groupBy'; | |||
|
|
|||
| import type {BaseAvatarProps} from '@sentry/scraps/avatar'; | |||
There was a problem hiding this comment.
BaseAvatar export from scraps was just bad naming.
| width: 100%; | ||
| `; | ||
|
|
||
| const StyledAvatar = styled(BaseAvatar)` |
There was a problem hiding this comment.
AvatarContainer already uses flexShrink: 0
| /** Defines the styling interface for all avatar components */ | ||
| export interface BaseAvatarStyleProps { | ||
| round?: boolean; | ||
| size?: number; |
There was a problem hiding this comment.
Added here so that all avatars inherit it. I will followup with converting this to t-shirt sizes.

Refactor avatar components to modular architecture
The old
BaseAvatarcomponent handled all avatar types in one place with a complex props interface that couldn't catch invalid prop combinations. The fallback logic propagated child updates back to parents through callbacks, making the flow indirect and hard to follow.This splits it into dedicated
LetterAvatar,Gravatar, andImageAvatarcomponents with a type-discriminatedAvatarentry point. The new API is explicit: bothImageAvatarandGravatardirectly renderLetterAvatarwhen images fail to load, no callback propagation needed. TypeScript discriminated unions prevent invalid prop combinations.The refactor makes it much clearer that
UserAvatar,TeamAvatar, andOrganizationAvatarare just thin wrappers that extract avatar data from their respective Sentry entities and pass it to the baseAvatarcomponent. Includes comprehensive test coverage and fixes 2 test failures relying on removedtitleattributes.