[No QA] Decompose Icon component#95048
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| import type {IconSize} from './types'; | ||
|
|
||
| /** Resolves the effective icon size from the new `size` prop or legacy boolean props. */ | ||
| function resolveIconSize(size: IconSize | undefined, extraSmall: boolean, small: boolean, medium: boolean, large: boolean): IconSize | undefined { |
There was a problem hiding this comment.
This function is a bit wonky, and it's not clear to me why it's helpful
There was a problem hiding this comment.
seeing how you're using it, it seems fine for now but we should create a follow-up to rip out the deprecated props
| import type {Dimensions} from '@src/types/utils/Layout'; | ||
|
|
||
| /** Tracks the measured layout size of the multi-gesture icon canvas. */ | ||
| function useCanvasSize() { |
There was a problem hiding this comment.
Maybe belongs better in the src/components/MultiGestureCanvas directory/"namespace".
| }; | ||
|
|
||
| /** Renders an SVG icon with preset sizes, inline layout, and optional gesture support. */ | ||
| function Icon({ |
There was a problem hiding this comment.
Overall this PR seems like a step in the right direction, but the Icon component is still kind of a configuration-overloaded barrel. Yes, we can use the primitives elsewhere now. But in practice this component still encapsulates all the configuration we had before. A complete decomposition refactor would be a bigger lift - getting rid of this wrapper/config barrel and using the appropriate primitives at each callsite.
I understand that's a larger refactor though, so it doesn't necessarily have to be part of this PR (especially since the primitives being available unblocks decomposition further up the tree)
There was a problem hiding this comment.
Yeah, both Icon/index and deprecated props I would like to address in follow ups, then what now is BaseIcon will become Icon and others will just be used directly.
|
@Pujan92 @roryabraham One of you needs to 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] |
|
It seems Melvin occasionally assigned two C+ here. @roryabraham Do we need C+ review here? I'll proceed C+ review if needed. |
|
ok, we'll let you work as C+ here @eh2077 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-07.at.5.03.09.PM.movAndroid: mWeb ChromeScreen.Recording.2026-07-07.at.4.45.45.PM.moviOS: HybridAppScreen.Recording.2026-07-07.at.5.00.06.PM.moviOS: mWeb SafariScreen.Recording.2026-07-07.at.4.17.29.PM.movMacOS: Chrome / SafariScreen.Recording.2026-07-07.at.4.19.02.PM.mov |
| style={StyleSheet.absoluteFill} | ||
| onLayout={updateCanvasSize} | ||
| > | ||
| {!isCanvasLoading && !!canvasSize && ( |
There was a problem hiding this comment.
NAB: I think we can use canvasSize only for simplicity.
| type IconSize = ValueOf<typeof CONST.ICON_SIZE>; | ||
|
|
||
| /** Shared render props for icon primitive components. */ | ||
| type BaseIconProps = { |
There was a problem hiding this comment.
I think we should include size?: IconSize; prop, right?
There was a problem hiding this comment.
nah, this is for primitives and they have resolved
iconWidth={iconWidth}
iconHeight={iconHeight}
passed
eh2077
left a comment
There was a problem hiding this comment.
tested well, just two minor comments above
@eh2077 all addressed |
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
roryabraham
left a comment
There was a problem hiding this comment.
approving and looking forward to the follow-ups discussed above
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.32-0 🚀
|
Help site review — no changes requiredI reviewed the changes in this PR against the help site content under Why: This is a purely internal, developer-facing refactor. It decomposes the
None of this changes any user-facing feature, workflow, setting, tab, label, or button. The PR is explicitly Since no help site changes are needed, I did not open a draft docs PR. @jmusial — no linked help site PR was created because this refactor has no user-facing impact. If you believe a specific article is affected, let me know which one and I'll take another look. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.32-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
This refactors the monolithic
Iconcomponent into focused primitives (BaseIcon,InlineIcon,MultiGestureIcon) plus small hooks, and adds a size prop backed byCONST.ICON_SIZEthat deprecates theextraSmall/small/medium/largebooleans.The
inline(3 call sites) andenableMultiGestureCanvas(1 call site —AttachmentView) branches are kept behavior-identical for backward compatibility.Given how few call sites use them, both can be migrated out of the
IconAPI into dedicated components in a future follow-up. Afterwards all otherIconcall sites can be migrated toBaseIcon.Fixed Issues
$ #95156
Tests
For regular icons
For inline icons
Copy→ label/icon toggle to the "copied" state, icon stays aligned.For multi gesture icons
Edit->View photoOffline tests
Same as tests.
QA Steps
N/A - Performance oriented refactor, not changing any functionality.
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-07-02.at.15.53.42.mov
Android: mWeb Chrome
Screen.Recording.2026-07-02.at.15.56.17.mov
iOS: Native
Screen.Recording.2026-07-02.at.15.29.24.mov
iOS: mWeb Safari
Screen.Recording.2026-07-02.at.15.30.36.mov
MacOS: Chrome / Safari