Repository navigation
MenuItem decomposition - description text style - #102263
roryabraham merged 18 commits into
Conversation
war-in
changed the title
MenuItem decomposition - description text styleMenuItem decomposition - description text style
…xt-style # Conflicts: # src/components/MenuItem/index.ts # src/components/MoneyRequestConfirmationList/sections/AttendeeField.tsx
war-in
changed the title
MenuItem decomposition - description text styleMenuItem decomposition - description text style
|
@Pujan92 PR's ready for review! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const {translate} = useLocalize(); | ||
| const styles = useThemeStyles(); | ||
| const {isLargeScreenWidth} = useResponsiveLayout(); | ||
| const {isLargeScreenWidth, isSmallScreenWidth} = useResponsiveLayout(); // eslint-disable-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
This line disables the rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth rule with eslint-disable-line but carries no comment explaining why isSmallScreenWidth is used here instead of the preferred shouldUseNarrowLayout. Unjustified disables hide the violation from the seatbelt baseline permanently, so a reviewer or future maintainer can't tell whether the suppression is intentional or an oversight.
Either add a justification comment explaining why the raw isSmallScreenWidth is genuinely needed here (e.g. the icon variant must key off the actual screen width, not the RHP-aware narrow-layout flag), or, if this is a temporary convenience, drop the disable and widen the baseline with SEATBELT_INCREASE instead so the violation stays visible. For example:
// isSmallScreenWidth is intentional here: the icon width must follow the real viewport, not the RHP narrow-layout flag
const {isLargeScreenWidth, isSmallScreenWidth} = useResponsiveLayout(); // eslint-disable-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidthReviewed at: f96ed56 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
…xt-style # Conflicts: # src/pages/workspace/rules/FlagForReviewRules/FlagForReviewRulePageBase.tsx # src/pages/workspace/rules/MerchantTypeRules/MerchantTypeRulePageBase.tsx # src/pages/workspace/rules/RequireFieldsRules/RequireFieldsRulePageBase.tsx
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-10-08.at.17.09.32.mov |
|
@war-in conflicts and some checks are failing |
…xt-style # Conflicts: # src/pages/DynamicReportDetailsPage/index.tsx # src/pages/workspace/recruiting/approver/MergeATSFinalApproverMenuItem.tsx
|
@Pujan92 done 🫡 |
| <BaseMenuItemIcon | ||
| src={src} | ||
| fill={iconFill} | ||
| additionalStyles={[styles.popoverMenuIcon, isCompactPopover && styles.wAuto]} |
There was a problem hiding this comment.
I didn't get this conversion, is it to prevent conditional styling?
There was a problem hiding this comment.
Yes, I split it so callers pick the width explicitly. DistanceCounter needs a narrow icon outside a compact popover, which the old leaf couldn't do
|
Perf test is failing |
|
@Pujan92 review addressed |
| </MenuItem.Trailing> | ||
| )} | ||
| </MenuItem.Row> | ||
| {shouldDisplayAttendeesError && ( |
There was a problem hiding this comment.
NAB/Follow-up: shouldDisplayAttendeesError is a configuration prop.
| <MenuItem.Leading> | ||
| <MenuItem.IconNarrow src={icons.Crosshair} /> | ||
| </MenuItem.Leading> | ||
| <MenuItemField.Content name={translate('common.distance')}> |
There was a problem hiding this comment.
NAB/unrelated follow-up: This code doesn't really work with internationalization. In some locales, we might not have a space between the amount and the unit, or the unit might come before the amount, or the amount and the unit might come before whatever common.distance is. This should really be a full phrase in en.ts and the other language files, with its parts styled richly as needed by react-native-render-html.
@MelvinBot create a follow-up issue for this. Assign to me, label Monthly.
There was a problem hiding this comment.
Created Make the GPS distance counter label a fully localized phrase. It's assigned to roryabraham and labeled Monthly.
The issue also notes that the unit is the raw mi / km value and is never translated.
|
✋ 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.5.6-0 🚀
|
|
No help site update needed. This PR only changes how existing rows are built and styled (font sizes, icon widths, accessibility announcements), and it doesn't change any feature, label, or workflow that an article describes. |
|
This PR failing because of the issue #103560 |
|
@jponikarchuk looking! |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.6-6 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
Migrates rows using
descriptionTextStyle/shouldIconUseAutoWidthStyleto the composableMenuItemAPI. Element values go inMenuItemField.Content, the one-off 15px names become 13px, and newMenuItem.IconNarrowreplaces auto-width icons. The a11y context splits label/announcement registries, so rows with an explicit label keep announcements.Fixed Issues
$ #100159
PROPOSAL:
Tests
Open a trip room with flight, train, car and hotel reservations and tap each reservation to open its details
Verify that take-off/landing, departs/arrives, pick-up/drop-off and check-in/check-out show a small field name above the large time, with the location text under the flight, train and car rows
Verify that the passenger/driver/guest row shows the name above the user pill, and that the hotel address and flight times can still be copied (hover button on web, long press on mobile)
Go to Workspace > Distance rates, open a rate
Verify that Name, Rate, Start date, End date and Tax reclaimable on show their field names at the same size as the Tax rate row
Go to Workspace > Rules, start creating a "Flag for review" rule and tap Save without picking a category
Verify that the Category row shows the folder icon right next to the text (no wide gap) and the "This field is required" error under the row
Go to Search and open the Display popup on a wide screen, then on a narrow screen
Verify that the "Edit columns" row has a compact icon on the wide screen and the regular icon on the narrow screen, and that tapping it opens the columns editor
With a screen reader on, create an expense in a workspace that requires attendees, clear the attendees and try to submit
Verify that the Attendees row is announced with its attendee names followed by "Your review is required"
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