refactor: MenuItem decomposition into a compound component - #96650
refactor: MenuItem decomposition into a compound component#96650war-in wants to merge 28 commits into
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b26bd03727
ℹ️ 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".
jmusial
left a comment
jmusial
left a comment
There was a problem hiding this comment.
This is just a first batch - will finish tomorrow
| const styles = useThemeStyles(); | ||
| const {isDisabled, isCompact} = useMenuItemState(); | ||
|
|
||
| return <View style={[styles.flexRow, styles.pointerEventsAuto, styles.gap3, isDisabled && styles.cursorDisabled, isCompact && styles.alignItemsCenter, style]}>{children}</View>; |
There was a problem hiding this comment.
Create named styles for componenets please - will look cleaner
| /** Container variant. `default` reserves the classic fixed-width cell; `compact` hugs the icon. | ||
| * Rows inside a compact menu are compact regardless of this prop. */ | ||
| variant?: MenuItemIconVariant; |
There was a problem hiding this comment.
Maybe “size” would be a better name
Guccio163
left a comment
Guccio163
left a comment
There was a problem hiding this comment.
A lot of work done, looks like we have a lot to go through; Here are some things that I've found, I'm open for a discussion.
BTW I feel like if you'd introduced some MenuItem-copy component that would in some way show which MenuItem fragments are being replaced with which primitives it would make comparison that easier, moreover we'd see in the migrated component which parts are still being used and need resolution. I would see it as a (f.ex.) nowhere-used MenuItemOld.tsx deprecated file next to the MenuItem.tsx, pushed before so it shows all changes, with code fragments replaced with exact primitives. LMKWYT 👀
JakubKorytko
left a comment
JakubKorytko
left a comment
There was a problem hiding this comment.
nice decomposition, the leaf split reads well and the compiled files pass the React Compiler compliance check. A couple of small cleanups below that are additive to the feedback already left by other reviewers (I skipped the points already raised: the missing index.ts, and the many-props-on-Root discussion).
jmusial
left a comment
jmusial
left a comment
There was a problem hiding this comment.
I would suggest a different, more step by step approach to this refactor.
Right now half of the new components have no call sites, but bloat out the PR making it harder to review and will lengthen whole process.
How about @ first implementing only the components used in a particular migration use case and then adding them progressively while migrating following call sites ?
Would make PRs smaller and redundant props easier to spot
…function for MenuItemIcon
5 participants
Explanation of Change
Decomposes the monolithic
MenuItem(~135 props) into a composable compound component. Consumers now assemble a row from sub-components (MenuItem.Row,.Icon,.Avatar,.Content,.Title,.Description,.Trailing,.Chevron,.BrickRoadIndicator, etc.), with interaction state (hover/press/focus/disabled) shared viaMenuItemContext. MovesMenuIteminto its own directory and migratesSettingsMenuItemto the new pattern as the first consumer.Fixed Issues
$ #96202
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari