[HOLD] ReportActionAvatars — Layer 1 primitives + IconsAvatar#94682
[HOLD] ReportActionAvatars — Layer 1 primitives + IconsAvatar#94682jmusial wants to merge 8 commits into
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@abzokhattab Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-06-30.at.02.36.29.moviOS: mWeb SafariScreen.Recording.2026-06-30.at.02.37.08.movMacOS: Chrome / SafariScreen.Recording.2026-06-30.at.02.35.02.mov |
|
|
||
| type DiagonalAvatarsProps = MultipleAvatarsProps & { | ||
| /** Whether to use the mid-subscript size for the avatars */ | ||
| useMidSubscriptSize: boolean; |
There was a problem hiding this comment.
Looking at this prop, it seems like it was added because we needed a size between SMALL and SMALLER. Overall I don't think that's a good component design choice - it's precisely the kind of configuration overload that makes components hard to reason about.
Instead, we should create a consistent size/scale guide for avatars.
However, looking at what we have today, we have over the years patched together some confusing semantics around this. Here's the full size ladder today:
| Constant | px |
|---|---|
| SMALL_SUBSCRIPT | 12 |
| MID_SUBSCRIPT / MENTION_ICON | 16 |
| SUBSCRIPT | 20 |
| SMALLER | 24 |
| SMALL | 28 |
| SMALL_NORMAL | 32 |
| HEADER | 40 |
| DEFAULT | 40 (same as HEADER) |
| LARGE_NORMAL | 48 |
| MEDIUM | 52 |
| MEDIUM_LARGE | 60 |
| LARGE | 80 |
| LARGE_BORDERED | 88 |
| X_LARGE | 100 |
yikes! We are mixing ordinal scale, usage-context, and hybrid names slotted between other sizes (ie.: LARGE_NORMAL is smaller than MEDIUM)
I suggest we standardize and refactor as necessary:
| Size | px |
|---|---|
| XXXXX_SMALL | 12 |
| XXXX_SMALL | 16 |
| XXX_SMALL | 20 |
| XX_SMALL | 24 |
| X_SMALL | 28 |
| SMALL | 32 |
| MEDIUM | 40 |
| LARGE | 48 |
| X_LARGE | 52 |
| XX_LARGE | 60 |
| XXX_LARGE | 80 |
| XXXX_LARGE | 88 |
| XXXXX_LARGE | 100 |
and can add aliases for things like DEFAULT or HEADER that reference the standard sized listed here. It also might make sense to do this in a separate PR before this one is merged.
cc @Expensify/design
There was a problem hiding this comment.
Interesting idea, I don't mind it! But things like this: LARGE_BORDERED: 88 only exists because we are using the large size (80px) plus a 4px border... so that means 80 + 4px on both sides = 88. What I am saying is, your scale would be misleading, because 88 size isn't actually XXXX_LARGE, it's just XXX_LARGE with added size for borders.
That makes me think others are doing similar things. For instance, I can't even tell you where we are using 52 or 60 size avatars - so maybe there are a few we could even kill!
There was a problem hiding this comment.
Thx @roryabraham, making avatar sizes consistent sounds like a great idea to me, I would suggest to do it in a separate PR. That refactor touches 70 + files.
Mapping table
| Old name | New name | px | Occurrences |
|---|---|---|---|
SMALL_SUBSCRIPT |
XXXXX_SMALL |
12 | 2 |
MID_SUBSCRIPT |
XXXX_SMALL |
16 | 18 |
MENTION_ICON |
XXXX_SMALL (merged) |
16 | 2 |
SUBSCRIPT |
XXX_SMALL |
20 | 2 |
SMALLER |
XX_SMALL |
24 | 7 |
SMALL |
X_SMALL |
28 | 24 |
SMALL_NORMAL |
SMALL |
32 | 4 |
HEADER |
MEDIUM |
40 | 1 |
DEFAULT |
DEFAULT (alias of MEDIUM) |
40 | 39 |
LARGE_NORMAL |
LARGE |
48 | 2 |
MEDIUM |
X_LARGE |
52 | 4 |
MEDIUM_LARGE |
XX_LARGE |
60 | 1 |
LARGE |
XXX_LARGE |
80 | 9 |
LARGE_BORDERED |
XXXX_LARGE |
88 | 4 |
X_LARGE |
XXXXX_LARGE |
100 | 30 |
| Total: 149 |
@shawnborton What we can potentially get rid of (using old names below):
SMALL_SUBSCRIPT,SUBSCRIPT,MEDIUM_LARGE- appear only as subscripts &SUBSCRIPTfor split bill preview (screen)
SMALL- in practice only used once, other 3 occurances are just checks for size. The header icon size (hardcoded it there, couldn't make it appear from the flow)
LARGE_NORMAL- only used insrc/components/OnboardingHelpDropdownButtonandsrc/pages/settings/Agents/AgentInfoRow.tsx
LARGE_BORDERED- used only when you have multiple avatars inRoomHeader(ex. Invoice send)
There was a problem hiding this comment.
There was a problem hiding this comment.
Definitely love the idea of cleaning some of these up - agree with Shawn that I bet a lot of these are unnecessary at this point, and even some that are being used could likely bump up or down a tiny bit to consolidate the size variables.
There was a problem hiding this comment.
Yeah agree with all of those points. For Agents, we should be able to kill that size once the Agents page starts using the new tables. For split, let's just reuse what we have for the attendee avatar stack that we use for the Attendees column on Spend/expense reports.
There was a problem hiding this comment.
For the others:
- one occurance of
SMALLcan I useDEFAULT? LARGE_BORDERED->X_LARGE?
There was a problem hiding this comment.
Yes to the first.
For the large bordered version, why do we need a version that includes the border width? Cant we define that separately? Like we need a large avatar that has 4px stroke applied to it, or whatever it is.
There was a problem hiding this comment.
I think it's just unfortunate naming. Most avatars have / allow for borders and they're calculated in the size. On example of LARGE (old name) the avatar would be 72, cos border width is 4.
code ref src/styles/utils/index.ts#144:
const avatarBorderWidths: Partial<Record<AvatarSizeName, number>> = {
[CONST.AVATAR_SIZE.DEFAULT]: 3,
[CONST.AVATAR_SIZE.SMALL_SUBSCRIPT]: 2,
[CONST.AVATAR_SIZE.MID_SUBSCRIPT]: 2,
[CONST.AVATAR_SIZE.SUBSCRIPT]: 2,
[CONST.AVATAR_SIZE.SMALL]: 2,
[CONST.AVATAR_SIZE.SMALLER]: 2,
[CONST.AVATAR_SIZE.HEADER]: 2,
[CONST.AVATAR_SIZE.LARGE]: 4,
[CONST.AVATAR_SIZE.MEDIUM_LARGE]: 3,
[CONST.AVATAR_SIZE.X_LARGE]: 4,
[CONST.AVATAR_SIZE.MEDIUM]: 3,
[CONST.AVATAR_SIZE.LARGE_BORDERED]: 4,
}
There was a problem hiding this comment.
Maybe the constants for avatar size should not account for borders, and instead we just add the 4px for border width in the component during rendering if there's a border
| isActive = false, | ||
| isPressed = false, | ||
| maxAvatarsInRow = CONST.AVATAR_ROW_SIZE.DEFAULT, | ||
| displayInRows: shouldDisplayAvatarsInRows = false, |
There was a problem hiding this comment.
make sure boolean variables (including props) start with is, should, has, etc...
But in this particular PR especially, take a close look at any boolean props and consider why or if they're needed, since often they represent configuration we're trying to get away from.
I think it's fine for things that represent controlled state, such as isHovered, isActive, isPressed. I haven't looked very closely, but shouldDisplayAvatarsInRows sounds like it could actually be a separate component
There was a problem hiding this comment.
Updated, most of the flags are used for styling @Expensify/design should we have 7 or more different border options for horizontally stacked avatars ?
code src/styles/utils/index.ts#874
function getHorizontalStackedAvatarBorderStyle({
theme,
isHovered,
isPressed,
isInReportAction = false,
shouldUseCardBackground = false,
isActive = false,
customPressedBorderColor,
}: AvatarBorderStyleParams): ViewStyle {
let borderColor = shouldUseCardBackground ? theme.cardBG : theme.appBG;
if (isHovered) {
borderColor = isInReportAction ? theme.hoverComponentBG : theme.border;
}
if (isActive) {
borderColor = theme.messageHighlightBG;
}
if (isPressed) {
borderColor = isInReportAction ? theme.hoverComponentBG : theme.buttonPressedBG;
if (customPressedBorderColor) {
borderColor = customPressedBorderColor;
}
}
return {borderColor};
}
A splitting over shouldDisplayAvatarsInRows I don't think HorizontalAvatars is a good case for that. it only drives the 2D array split, the rest of the component logic is same. Could move it into an util and get rid of the flag though ?
| /** `ProfileAvatar` wraps an `Avatar` in a pressable that navigates to the correct "view avatar" route. | ||
| * The branch it picks depends on `type` (workspace vs user) and whether a `reportID` is provided. | ||
| */ | ||
| function ProfileAvatar({shouldUseProfileNavigationWrapper, ...props}: ProfileAvatarProps) { |
There was a problem hiding this comment.
This is a perfect example of a boolean prop that should be removed in favor of composition.
Rather than having:
<ProfileAvatar shouldUseProfileNavigationWrapper />turn into:
<PressableWithoutFeedback {...someProps}>
<Avatar />
</PressableWithoutFeedbackWe should create a <ProfileNavigationWrapper /> component, and then compose it like so:
<ProfileAvatar.NavigationWrapper>
<ProfileAvatar />
</ProfileAvatar.NavigationWrapper>|
|
||
| const {avatarID, type, name, reportID} = props; | ||
|
|
||
| const isWorkspace = type === CONST.ICON_TYPE_WORKSPACE; |
There was a problem hiding this comment.
I notice that since this is just wrapping Avatar, and Avatar has forking logic due to type, it's breaking down composition further up the tree.
What we've learned with these component decompositions is to actually achieve the end result we want, it's necessary to start from the bottom of the tree and work our way up.
So we should:
- Put this PR on HOLD
- Create a separate PR off main to split the lower-level Avatar component into
AvatarandWorkspaceAvatar - After that's merged, pick this PR back up, removing
typeprops and the forkingisWorkspacelogic.
There was a problem hiding this comment.
Fair point, putting this one on HOLD. While checking this I noticed Icon component (Avatar uses it) could benefit from decomposition as well. Let me know what you think @roryabraham.
Draft PR for Icon refactor here.
Draft PR for Avatar refactor here, but would like to do above first.
cc. @mountiny as Icon and Avatar decomposition would need separate sub issue / or full issue if we consider migration or current call sites later.
There was a problem hiding this comment.
thanks, looking at those others. I think we'll definitely be best off starting from the bottom and working our way up
Explanation of Change
This PR extracts Avatar primitives into separate file structure and unifies
LHNAvatarandSearchReportAvatarinto 1 thin adapter.Fixed Issues
$ #94585
PROPOSAL:
slack
Tests
This PR is pure refactor:
A. Verify
ExpenseReportListItemAvatarOffline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2026-06-29.at.09.55.56.mov
Android: mWeb Chrome
Screen.Recording.2026-06-29.at.09.56.40.mov
iOS: Native
Screen.Recording.2026-06-29.at.09.47.38.mov
iOS: mWeb Safari
Screen.Recording.2026-06-29.at.09.49.18.mov
MacOS: Chrome / Safari
Screen.Recording.2026-06-29.at.09.38.54.mov