[No QA] refactor: decompose OptionRowLHN alternate text and action badge#89274
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
BartekObudzinski
changed the title
melvin-bot
Bot
requested review from
joekaufmanexpensify and
stitesExpensify
and removed request for
a team
|
@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
JS00001
left a comment
There was a problem hiding this comment.
Please remove mobile-expensify changes
| import type {Report} from '@src/types/onyx'; | ||
| import LHNTooltipContextProvider from './LHNTooltipContextProvider'; | ||
| import OptionRowLHNData from './OptionRowLHNData'; | ||
| /** Row package entry: OptionRowLHN barrel exports OptionRowLHNData only. */ |
There was a problem hiding this comment.
| /** Row package entry: OptionRowLHN barrel exports OptionRowLHNData only. */ |
There was a problem hiding this comment.
Done — comment line removed.
|
|
||
| /** LHN row press: telemetry span, composer focus, then onSelectRow. Lives next to OptionRowPressable per review. */ | ||
| function useOptionRowLHNCorePress({reportID, optionItem, popoverAnchor, onSelectRow}: UseOptionRowLHNCorePressParams): OptionRowLHNCorePressHandler { | ||
| return useCallback( |
There was a problem hiding this comment.
No need to use useCallback because of the new React Compiler usage
There was a problem hiding this comment.
Removed. Verified the file still compiles with React Compiler — and OptionRowPressable isn't on the LHN memoization-exception list, so the compiler handles it.
| @@ -45,12 +48,10 @@ function OptionRowLHNData({ | |||
|
|
|||
| const oneTransactionThreadReportID = oneTransactionThreadReport?.reportID; | |||
|
|
|||
| // Per-item report actions subscriptions (scoped by specific report ID) | |||
There was a problem hiding this comment.
Could you clarify what you'd like me to look at? Line 48 is const {login, accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); — both values are used downstream (currentUserAccountID and login/currentUserLogin are passed into getOptionData), so the destructure is intentional. Let me know if you meant something else.
There was a problem hiding this comment.
Not intentional — that was the // Per-item report actions subscriptions (scoped by specific report ID) line above the useOnyx(REPORT_ACTIONS...) block. Restored.
| onLayout={onLayout} | ||
| accessibilityLabel={accessibilityLabelWithContextMenuHint} | ||
| accessibilityHint={accessibilityHint} | ||
| // reportID may be a number contrary to the type definition |
There was a problem hiding this comment.
Is it intentional to remove the comment?
There was a problem hiding this comment.
Not intentional — that was the // reportID may be a number contrary to the type definition line above the testID={...} cast. Restored.
|
Updated @DylanDylann lets wait for ci |
| popoverAnchor={popoverAnchor} | ||
| onPress={onPress} | ||
| onSelectRow={onSelectRow} | ||
| onPressBefore={onPressBefore} |
There was a problem hiding this comment.
Why do we need onPressBefore prop?
There was a problem hiding this comment.
To preserve the existing tooltip-hide-on-press behavior.
There was a problem hiding this comment.
@BartekObudzinski onPressBefore is currently just hideProductTrainingTooltip. I suggest removing the onPressBefore prop and defining hideProductTrainingTooltip directly inside OptionRowPressable. If a prop can be defined within the child component itself, we should avoid passing it down unnecessarily.
There was a problem hiding this comment.
Please put
const {shouldUseNarrowLayout} = useResponsiveLayout();
const {isFullscreenVisible, isScreenFocused, isReportsSplitNavigatorLast} = useLHNTooltipContext();
const {tooltipToRender, shouldShowTooltip} = useMemo(() => {
// TODO: CONCIERGE_LHN_GBR tooltip will be replaced by a tooltip in the #admins room
// https://github.com/Expensify/App/issues/57045#issuecomment-2701455668
const tooltip = CONST.PRODUCT_TRAINING_TOOLTIP_NAMES.CONCIERGE_LHN_GBR;
const shouldTooltipBeVisible = shouldUseNarrowLayout ? isScreenFocused && isReportsSplitNavigatorLast : isReportsSplitNavigatorLast && !isFullscreenVisible;
return {
tooltipToRender: tooltip,
shouldShowTooltip: shouldTooltipBeVisible,
};
}, [isScreenFocused, shouldUseNarrowLayout, isReportsSplitNavigatorLast, isFullscreenVisible]);
const {shouldShowProductTrainingTooltip, renderProductTrainingTooltip, hideProductTrainingTooltip} = useProductTrainingContext(tooltipToRender, shouldShowTooltip);
into a new hook and using it in both OptionRowTooltipLayerInner and OptionRowPressable
There was a problem hiding this comment.
Done, extracted into useLHNRowProductTrainingTooltip
| * This component gets the data from onyx for the actual | ||
| * OptionRowLHN component. | ||
| * The OptionRowLHN component is memoized, so it will only |
There was a problem hiding this comment.
I noticed many comments were removed in this PR. Could you restore them if it wasn’t intentional?
| const corePress = useOptionRowLHNCorePress({reportID, optionItem, popoverAnchor, onSelectRow}); | ||
| const onPress = (event: GestureResponderEvent | KeyboardEvent | undefined) => { | ||
| onPressBefore?.(); | ||
| corePress(event); |
There was a problem hiding this comment.
Could we move
startSpan(`${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${reportID}`, {
name: 'OptionRowLHN',
op: CONST.TELEMETRY.SPAN_OPEN_REPORT,
});
event?.preventDefault();
ReportActionComposeFocusManager.focus();
onSelectRow(optionItem, popoverAnchor);
into here instead of defining corePress function
|
@DylanDylann answered on comments |
|
@DylanDylann ready |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ 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". |
|
@JS00001 ready |
|
^ those pr will be opened if we merged this one becasue its blocking |
|
🚧 @JS00001 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. |
7 participants
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