[No QA] refactor: decompose OptionRowLHN alternate text and action badge#89274
[No QA] refactor: decompose OptionRowLHN alternate text and action badge#89274BartekObudzinski wants to merge 11 commits intoExpensify:mainfrom
Conversation
… child components
|
in case we want to ship it there is a holiday tomorrow (01.05) and i will be back on monday |
|
No c+ is needed here |
…option-row-lhn # Conflicts: # src/components/LHNOptionsList/OptionRowLHN.tsx
|
@stitesExpensify Please 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] |
|
@BartekObudzinski Please revert Mobile-Expensify folder |
|
All good. But I suggest creating an OptionRowLHN folder, then put all child components in this folder. |
Reviewer Checklist
Screenshots/VideosVerified the RBR/GBR and the alternative text display correctly
|
JS00001
left a comment
There was a problem hiding this comment.
Please remove mobile-expensify changes
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
LGTM, lint is failing though. @codex review please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 052f36a906
ℹ️ 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".
37a4c1e
|
|
||
| OptionRowTooltipLayerInner.displayName = 'OptionRowTooltipLayerInner'; | ||
|
|
||
| function OptionRowTooltipLayer({shouldShowRBRorGBRTooltip, shouldShowGetStartedTooltip, onOptionPress, renderChildren}: OptionRowTooltipLayerProps) { |
There was a problem hiding this comment.
@BartekObudzinski Is there a reason we need to pass these props? Could this component define them?
| @@ -331,16 +289,12 @@ function OptionRowLHN({ | |||
| shouldShowErrorMessages={false} | |||
| needsOffscreenAlphaCompositing | |||
| > | |||
There was a problem hiding this comment.
Please move this wrapper into OptionRowTooltipLayer
|
@BartekObudzinski Should we move OptionRowLHNData to the new folder (may change the folder name if needed for importing) |
| actionBadgeText: string; | ||
| }; | ||
|
|
||
| function OptionRowActionBadge({severity, actionBadgeText}: OptionRowActionBadgeProps) { |
There was a problem hiding this comment.
Please refactor to avoid duplicated code. I’m fine with creating two separate components instead of combining them like this.
| @@ -254,41 +226,20 @@ | |||
| </View> | |||
| ) : null} | |||
| {brickRoadIndicator === CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR && ( | |||
There was a problem hiding this comment.
Please move this check into the child component
| @@ -227,22 +210,11 @@ | |||
| )} | |||
| </View> | |||
| {!!optionItem.alternateText && ( | |||
There was a problem hiding this comment.
Please move this check into child component
| if (!optionItem) { | ||
| // This is the case when the component is focused and the optionItem is null. | ||
| // For example, when you submit an expense in offline mode and click on the | ||
| // generated expense report, we would only see the Report Details but no item in LHN. | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Please move this null check to the parent component
|
No product review needed |

Explanation of Change
Extracts two self-contained subtrees out of
OptionRowLHNinto dedicated child components, mirroring the pattern from #89087 (OptionRowAvatar):OptionRowAlternateTextowns the alternate-text<Text>plus the custom-emoji branch (containsCustomEmoji+containsOnlyCustomEmoji+TextWithEmojiFragment). Removes theuseMemoforalternateTextContainsCustomEmojiWithTextfrom the parent.OptionRowActionBadgeowns the RBR/GBR badge JSX (theBadge+DotIndicatorIcon variants). Owns its ownuseTheme/useThemeStylesand readsDotIndicatorviauseMemoizedLazyExpensifyIconsso the parent's lazy-icon set shrinks from['Pencil', 'DotIndicator', 'Pin']to['Pencil', 'Pin'].The parent
OptionRowLHNkeeps itsReact.memowrapper and load-bearing manual memoization per the React Compiler RC-6 exception. Both new components compile cleanly with React Compiler.Measured impact
Perf-neutral.
ManualNavigateToInboxTabmeasurements on Android are within noise of main; this is a readability/composition refactor, not a perf win. Shipping it as prep so future extractions in this file (e.g. status badge) start from a smaller parent.Fixed Issues
$ #89275
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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