Repository navigation
MenuItemWithTopDescription decomposition - simplest usages - #100191
roryabraham merged 28 commits into
Conversation
…-item-with-top-description-decomposition
…-item-with-top-description-decomposition
…-item-with-top-description-decomposition # Conflicts: # src/pages/workspace/expensifyCard/issueNew/ConfirmationStep.tsx
…osition # Conflicts: # src/stories/MenuItemComparison.stories.tsx
…osition # Conflicts: # src/pages/settings/Wallet/InternationalDepositAccount/subPages/Confirmation.tsx
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review @MelvinBot review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
🤖 Code review of the The component work is solid: 🔴 Empty-value guards dropped around interpolated
|
JakubKorytko
left a comment
There was a problem hiding this comment.
LGTM, one dropped label and two small ones
|
@Pujan92 all yours 😄 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-09-09.at.14.34.16.movScreen.Recording.2026-09-09.at.14.25.26.movScreen.Recording.2026-09-09.at.14.22.20.mov |
|
@war-in let's merge main and fix conflicts |
| <MenuItem.Root onPress={!isBankAccountAdded ? handleModifyAccountNumbers : undefined}> | ||
| <MenuItem.Row> | ||
| <MenuItem.Content> | ||
| {!!bankName && <MenuItem.FieldName>{bankName}</MenuItem.FieldName>} | ||
| <MenuItem.FieldValue>{`${translate('bankAccount.accountEnding')} ${accountNumber.slice(-4)}`}</MenuItem.FieldValue> | ||
| </MenuItem.Content> |
There was a problem hiding this comment.
can't we use MenuItemField here?
There was a problem hiding this comment.
MenuItemField requires the title. Here, title can be undefined, so we would have to make the name optional only for this usage, which doesn't seem reasonable
Yes! I discussed this with Shawn and he agreed that the icon should have some space around it |
…osition # Conflicts: # src/components/MenuItem/index.ts # src/components/MenuItem/leaves/content/base/MenuItemLabel.tsx # src/pages/DynamicReportParticipantDetailsPage.tsx # src/pages/domain/Groups/PreferredWorkspaceToggle.tsx # src/pages/workspace/reports/CreateReportFieldsPage.tsx # src/pages/workspace/reports/ReportFieldsValueSettingsPage.tsx
|
@Pujan92 review addressed |
|
🚧 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.75-0 🚀
|
No help site changes requiredI reviewed all 78 changed files. Nothing in Why: this is a presentational refactor of the What I checkedNo copy changed. Zero files under Representative call site — The three behavioral notes in the PR body are all invisible to the help site:
Affected surfaces are all already documented accurately. The migrated screens include Notification preferences, workspace Categories / Tags / Members / Per diem / Company cards, Vacation delegate, and the accounting integration pages. Since the rendered text and navigation on each of those rows is unchanged, the existing articles still match the live UI. @war-in, since this refactor introduces no user-facing change, there is no linked help site PR to review. If you disagree — for example if a row's label or behavior did shift in a way I missed — reply with |
|
Hey @war-in. Is it an internal PR? |
@IuliiaHerets I'm not sure if I understand, could you clarify? |
|
@war-in We can follow only steps 4–8. The other steps are out of our scope. |
|
@IuliiaHerets That's what I mentioned in the PR description 😄 But can't you test |
|
@war-in VoiceOver & TalkBack is out of the scope of our work |
|
@IuliiaHerets oh, I didn't know that. So I think it's fine to just test 4-8 |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |

Explanation of Change
Phase 3 of the
MenuItemdecomposition:Title,Description,FieldName,FieldNamePlaceholderandFieldValue, over two shared bases (BaseMenuItemPrimaryText,BaseMenuItemSupportingText).top/bottom) instead of role, so the announced order matches the visual one for both field and navigation rows.MenuItemEmptyFieldwith aMenuItemFieldpreset that renders both the filled (name+value) and empty (nameonly) shapes.MenuItemWithTopDescriptioncall sites across 55 screens toMenuItemField.Phase 3section to theMenuItemComparisonstory, one card per prop shape.Fixed Issues
$ #100312
PROPOSAL:
Tests
Open Storybook (
npm run storybook) and go to the MenuItem Comparison story.Scroll to the
Phase 3 — MenuItemWithTopDescriptionsection.Verify that for every card the legacy
MenuItemWithTopDescriptioncolumn, the composable column, and the preset column render identically — same font size, weight, line height, spacing, chevron placement, and truncation.Filled field rows: navigate to Settings > Profile > (a chat) > Notification preferences row, Workspace > Categories > (a category) settings, Workspace > Tags > (a tag) settings, Workspace > Members > (a member) details, and Workspace > Per diem > (a rate) details.
Verify that each row shows the small grey field name on the top line and the full-contrast value below it, with a chevron when the row is pressable.
Tap the rows and verify navigation still works, and that non-interactive rows (e.g. Travel > Trip details > Car type) show no chevron and do not respond to taps.
Empty field rows: go to Settings > Preferences > Vacation delegate with no delegate set, and Workspace > Company cards > Assign card > Confirmation with an unfilled field.
Verify that the field name renders at normal (not small) font size and takes over the row, with the chevron on the right.
Turn on VoiceOver (iOS) / TalkBack (Android) and focus one filled row and one empty row.
Verify that the row is announced as a single element reading the top line first then the bottom line (e.g. "Notification preferences, Daily"), and the empty row announces just the field name.
Offline tests
N/A — presentational refactor only; no API calls or Onyx writes were changed.
QA Steps
Same as tests (steps 4-10; the Storybook step is dev-only).
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