[No QA] Unify avatar sizes#95275
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0787653f3d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! 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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
|
@ChavdaSachin 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] |
|
cc. @roryabraham the unification of |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79b05124e1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| <ReportActionAvatars | ||
| reportID={report?.reportID} | ||
| size={shouldShowSubscript ? defaultSubscriptSize : undefined} | ||
| singleAvatarContainerStyle={[styles.actionAvatar, styles.mr3]} |
There was a problem hiding this comment.
Restore the fallback 32px expense-thread avatar size
When an expense-request thread is opened before its parent report action collection is available, ReportHeader computes isReportTransactionThread(report) as false and mounts HeaderView; ReportHeader does not subscribe to that parent action, while HeaderView does via useParentReportAction, so this component can stay mounted after shouldReportShowSubscript() becomes true. Without the removed isExpenseRequest(report) size override, those direct/deeplinked expense-thread headers render the subscript avatar at the default 40px size instead of the previous 32px size, causing a visible header layout regression. Please keep the 32px mapping here or make the header variant selection reactive to the parent action.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removing 32px size is intentional per comment here
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product.
roryabraham
left a comment
There was a problem hiding this comment.
Overall huge fan of this change. It's definitely bit tricky to sort through, so thank you for doing it! Much more orderly now.
| [CONST.AVATAR_SIZE.XX_LARGE]: variables.componentBorderRadiusLarge, | ||
| [CONST.AVATAR_SIZE.XXX_LARGE]: variables.componentBorderRadiusLarge, | ||
| [CONST.AVATAR_SIZE.XXXX_LARGE]: variables.componentBorderRadiusLarge, | ||
| } satisfies Record<AvatarSizeName, number>; |
There was a problem hiding this comment.
Should this be:
| } satisfies Record<AvatarSizeName, number>; | |
| } as const satisfies Record<AvatarSizeName, number>; |
?
| [CONST.AVATAR_SIZE.XX_LARGE]: variables.avatarSizeXxLarge, | ||
| [CONST.AVATAR_SIZE.XXX_LARGE]: variables.avatarSizeXxxLarge, | ||
| [CONST.AVATAR_SIZE.XXXX_LARGE]: variables.avatarSizeXxxxLarge, | ||
| } satisfies Record<AvatarSizeName, number>; |
There was a problem hiding this comment.
Same here, should it be:
| } satisfies Record<AvatarSizeName, number>; | |
| } as const satisfies Record<AvatarSizeName, number>; |
?
| avatarBorderWidthSmall: 2, | ||
| avatarBorderWidthDefault: 3, | ||
| avatarBorderWidthLarge: 4, | ||
| }; |
There was a problem hiding this comment.
| }; | |
| } as const; |
?
|
@ChavdaSachin awaiting C+ review on this PR |
| assigneeTextStyle: { | ||
| ...FontUtils.fontFamily.platform.EXP_NEUE_BOLD, | ||
| minHeight: variables.avatarSizeSubscript, | ||
| minHeight: variables.avatarSizeXxxSmall, |
There was a problem hiding this comment.
This is inconsistent; the previous size avatarSizeSubscript resolved to 20px, while the current size avatarSizeXxxSmall resolves to 16px.
| minHeight: variables.avatarSizeXxxSmall, | |
| minHeight: variables.avatarSizeXxSmall, |
Explanation of Change
Replaces the legacy ad-hoc avatar sizes (
SMALL_NORMAL,HEADER,LARGE_BORDERED, …) with a linearxxxx-small(12px) →xxxx-large(100px) scale backed by centralized tokens and exhaustive style maps. Unifies how avatar borders are handled.Visual changes
None - all sizes are pixel-identical. The two spots where new defaults would have differed (subscript border on 100px avatars, workspace avatar radius in
RoomHeaderAvatars) keep their old values explicitly. (AttendeesCell's stacked-row container now includes the avatar border in its height for consistency with other stacks — layout-neutral, row height is padding-driven.)Dead code
Removed
HeaderView's 32pxSMALL_NORMALsubscript branch — unreachable, sinceReportHeadersends all expense-request reports toMoneyRequestHeaderfirst.Fixed Issues
$ #95598
PROPOSAL:
Coming from this thread
Tests
Refactor mostly without impact for the user, some basic tests:
Offline tests
Same as tests
QA Steps
N/A
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
MacOS: Chrome / Safari
Screen.Recording.2026-07-07.at.16.34.32.mov