MenuItem decomposition - simplest usages - #97339
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: 21764ae4a6
ℹ️ 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".
|
@codex review |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
This reverts commit a8b9024.
JakubKorytko
left a comment
JakubKorytko
left a comment
There was a problem hiding this comment.
the compound direction is right and the move itself is clean: 97% similarity rename, seatbelt path updated, compliance passes on all 11 new files, and none of the ~45 migrated call sites drop a prop the preset can't express.
Main thing: the anonymous-session guard stopped being the default. Legacy handed it to every caller (MenuItem.tsx:587), now only MenuItem.Standard applies it while the index.ts doc block tells people to compose Root directly. Details on the onPressAction line.
Second: isCompact merges two legacy notions that were deliberately separate. isCompactPopoverItem (MenuItem.tsx:656) and isCompact = viewMode === COMPACT (MenuItem.tsx:658) fed different helpers, and the context only carries one flag, so getMenuItemTextContainerStyle now gets the popover one and minHeight goes 40 to 20 inside a CompactMenuContext. Root also swaps isSmallScreenWidth for shouldUseNarrowLayout, which legacy suppressed a lint rule to avoid.
Third: the a11y label registry. Children pushing text up into parent state costs an extra subtree render per row, leaves accessibilityLabel="" on first paint, and the "render order" the docs promise is really mount order. Legacy computed it synchronously at MenuItem.tsx:669.
Also: 8 of the 10 test files that mock @components/MenuItem still return a bare function, so .Standard is undefined in them. They pass because the migrated rows sit behind conditions those tests don't hit, so it's a part 2 landmine rather than a break now
…by only one call site
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a4c4b7e4c
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
2 participants
Explanation of Change
Part 1 of the
MenuItemdecomposition. Moves the legacy monolith intosrc/components/MenuItem/and introduces a compound API (Root/Row/Content/Icon/Title/Description/Trailing/Chevron) sharing interaction state via context, plus a11y-label derivation fromTitle/Description. Adds aMenuItem.Standardpreset and migrates the simplest call sites to it. Legacy default export unchanged.Legacy
isInteractiveprop is derived fromonPress, so callers should pass undefined when row is not interactiveFixed Issues
$
PROPOSAL:
Tests
MenuItem.Standard(e.g. Profile, Share Code, Two-Factor Auth Enabled, Expensify Card, Workspace tags import).Offline tests
N/A — pure component refactor, no API or Onyx changes.
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