-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[No QA] Optimize ExpenseReportListItemRow - Split ActionCell into smaller components #83079
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
luacmartins
merged 5 commits into
Expensify:main
from
software-mansion-labs:perf/split-action-cell
Feb 23, 2026
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
9712947
Split ActionCell into smaller components
jmusial 987e2e0
address review findings
jmusial c643ba3
Merge branch 'main' into perf/split-action-cell
jmusial a0440b2
Merge branch 'main' into perf/split-action-cell
jmusial 158f7d8
fix prettier
jmusial File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
201 changes: 0 additions & 201 deletions
201
src/components/SelectionListWithSections/Search/ActionCell.tsx
This file was deleted.
Oops, something went wrong.
58 changes: 58 additions & 0 deletions
58
src/components/SelectionListWithSections/Search/ActionCell/BadgeActionCell.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import React from 'react'; | ||
| import {View} from 'react-native'; | ||
| import Badge from '@components/Badge'; | ||
| import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; | ||
| import useLocalize from '@hooks/useLocalize'; | ||
| import useStyleUtils from '@hooks/useStyleUtils'; | ||
| import useTheme from '@hooks/useTheme'; | ||
| import useThemeStyles from '@hooks/useThemeStyles'; | ||
| import variables from '@styles/variables'; | ||
| import CONST from '@src/CONST'; | ||
| import type {SearchTransactionAction} from '@src/types/onyx/SearchResults'; | ||
| import actionTranslationsMap from './actionTranslationsMap'; | ||
|
|
||
| type BadgeActionCellProps = { | ||
| action: SearchTransactionAction; | ||
| isLargeScreenWidth: boolean; | ||
| isSelected: boolean; | ||
| extraSmall: boolean; | ||
| shouldDisablePointerEvents?: boolean; | ||
| }; | ||
|
|
||
| function BadgeActionCell({action, isLargeScreenWidth, isSelected, extraSmall, shouldDisablePointerEvents}: BadgeActionCellProps) { | ||
| const {translate} = useLocalize(); | ||
| const theme = useTheme(); | ||
| const styles = useThemeStyles(); | ||
| const StyleUtils = useStyleUtils(); | ||
| const expensifyIcons = useMemoizedLazyExpensifyIcons(['Checkmark', 'Checkbox']); | ||
| const text = translate(actionTranslationsMap[action]); | ||
|
|
||
| return ( | ||
| <View | ||
| style={[StyleUtils.getHeight(variables.h20), styles.justifyContentCenter, shouldDisablePointerEvents && styles.pointerEventsNone]} | ||
| accessible={!shouldDisablePointerEvents} | ||
| accessibilityState={{disabled: shouldDisablePointerEvents}} | ||
| > | ||
| <Badge | ||
| text={text} | ||
| icon={action === CONST.SEARCH.ACTION_TYPES.DONE ? expensifyIcons.Checkbox : expensifyIcons.Checkmark} | ||
| badgeStyles={[ | ||
| styles.ml0, | ||
| styles.ph2, | ||
| styles.gap1, | ||
| isLargeScreenWidth ? styles.alignSelfCenter : styles.alignSelfEnd, | ||
| StyleUtils.getHeight(variables.h20), | ||
| StyleUtils.getMinimumHeight(variables.h20), | ||
| isSelected ? StyleUtils.getBorderColorStyle(theme.buttonHoveredBG) : StyleUtils.getBorderColorStyle(theme.border), | ||
| ]} | ||
| textStyles={StyleUtils.getFontSizeStyle(extraSmall ? variables.fontSizeExtraSmall : variables.fontSizeSmall)} | ||
| iconStyles={styles.mr0} | ||
| success | ||
| shouldUseXXSmallIcon={extraSmall} | ||
| /> | ||
| </View> | ||
| ); | ||
| } | ||
|
|
||
| export type {BadgeActionCellProps}; | ||
| export default BadgeActionCell; |
85 changes: 85 additions & 0 deletions
85
src/components/SelectionListWithSections/Search/ActionCell/PayActionCell.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| import React from 'react'; | ||
| import type {ValueOf} from 'type-fest'; | ||
| import {useDelegateNoAccessActions, useDelegateNoAccessState} from '@components/DelegateNoAccessModalProvider'; | ||
| import type {PaymentMethod} from '@components/KYCWall/types'; | ||
| import {SearchScopeProvider} from '@components/Search/SearchScopeProvider'; | ||
| import SettlementButton from '@components/SettlementButton'; | ||
| import useNetwork from '@hooks/useNetwork'; | ||
| import useOnyx from '@hooks/useOnyx'; | ||
| import usePolicy from '@hooks/usePolicy'; | ||
| import useReportWithTransactionsAndViolations from '@hooks/useReportWithTransactionsAndViolations'; | ||
| import useThemeStyles from '@hooks/useThemeStyles'; | ||
| import {canIOUBePaid} from '@libs/actions/IOU'; | ||
| import {getPayMoneyOnSearchInvoiceParams, payMoneyRequestOnSearch} from '@libs/actions/Search'; | ||
| import {convertToDisplayString} from '@libs/CurrencyUtils'; | ||
| import {isInvoiceReport} from '@libs/ReportUtils'; | ||
| import CONST from '@src/CONST'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import ROUTES from '@src/ROUTES'; | ||
|
|
||
| type PayActionCellProps = { | ||
| isLoading: boolean; | ||
| policyID: string; | ||
| reportID: string; | ||
| hash?: number; | ||
| amount?: number; | ||
| extraSmall: boolean; | ||
| shouldDisablePointerEvents?: boolean; | ||
| }; | ||
|
|
||
| function PayActionCell({isLoading, policyID, reportID, hash, amount, extraSmall, shouldDisablePointerEvents}: PayActionCellProps) { | ||
| const styles = useThemeStyles(); | ||
| const {isOffline} = useNetwork(); | ||
| const {isDelegateAccessRestricted} = useDelegateNoAccessState(); | ||
| const {showDelegateNoAccessModal} = useDelegateNoAccessActions(); | ||
| const [iouReport, transactions] = useReportWithTransactionsAndViolations(reportID); | ||
| const policy = usePolicy(policyID); | ||
| const [bankAccountList] = useOnyx(ONYXKEYS.BANK_ACCOUNT_LIST); | ||
| const [chatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${iouReport?.chatReportID}`); | ||
| const canBePaid = canIOUBePaid(iouReport, chatReport, policy, bankAccountList, transactions, false); | ||
| const shouldOnlyShowElsewhere = !canBePaid && canIOUBePaid(iouReport, chatReport, policy, bankAccountList, transactions, true); | ||
|
|
||
| const {currency} = iouReport ?? {}; | ||
|
|
||
| const confirmPayment = (type: ValueOf<typeof CONST.IOU.PAYMENT_TYPE> | undefined, payAsBusiness?: boolean, methodID?: number, paymentMethod?: PaymentMethod | undefined) => { | ||
| if (!type || !reportID || !hash || !amount) { | ||
| return; | ||
| } | ||
|
|
||
| if (isDelegateAccessRestricted) { | ||
| showDelegateNoAccessModal(); | ||
| return; | ||
| } | ||
|
|
||
| const invoiceParams = getPayMoneyOnSearchInvoiceParams(policyID, payAsBusiness, methodID, paymentMethod); | ||
| payMoneyRequestOnSearch(hash, [{amount, paymentType: type, reportID, ...(isInvoiceReport(iouReport) ? invoiceParams : {})}]); | ||
| }; | ||
|
|
||
| return ( | ||
| <SearchScopeProvider isOnSearch={false}> | ||
| <SettlementButton | ||
| shouldUseShortForm | ||
| buttonSize={extraSmall ? CONST.DROPDOWN_BUTTON_SIZE.EXTRA_SMALL : CONST.DROPDOWN_BUTTON_SIZE.SMALL} | ||
| currency={currency} | ||
| formattedAmount={convertToDisplayString(Math.abs(iouReport?.total ?? 0), currency)} | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| policyID={policyID || iouReport?.policyID} | ||
| iouReport={iouReport} | ||
| chatReportID={iouReport?.chatReportID} | ||
| enablePaymentsRoute={ROUTES.ENABLE_PAYMENTS} | ||
| onPress={(type, payAsBusiness, methodID, paymentMethod) => confirmPayment(type as ValueOf<typeof CONST.IOU.PAYMENT_TYPE>, payAsBusiness, methodID, paymentMethod)} | ||
| style={[styles.w100, shouldDisablePointerEvents && styles.pointerEventsNone]} | ||
| wrapperStyle={[styles.w100]} | ||
| shouldShowPersonalBankAccountOption={!policyID && !iouReport?.policyID} | ||
| isDisabled={isOffline || shouldDisablePointerEvents} | ||
| shouldStayNormalOnDisable={shouldDisablePointerEvents} | ||
| isLoading={isLoading} | ||
| onlyShowPayElsewhere={shouldOnlyShowElsewhere} | ||
| sentryLabel={CONST.SENTRY_LABEL.SEARCH.ACTION_CELL_PAY} | ||
| /> | ||
| </SearchScopeProvider> | ||
| ); | ||
| } | ||
|
|
||
| export type {PayActionCellProps}; | ||
| export default PayActionCell; | ||
15 changes: 15 additions & 0 deletions
15
src/components/SelectionListWithSections/Search/ActionCell/actionTranslationsMap.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import CONST from '@src/CONST'; | ||
| import type {TranslationPaths} from '@src/languages/types'; | ||
| import type {SearchTransactionAction} from '@src/types/onyx/SearchResults'; | ||
|
|
||
| const actionTranslationsMap: Record<SearchTransactionAction, TranslationPaths> = { | ||
| [CONST.SEARCH.ACTION_TYPES.VIEW]: 'common.view', | ||
| [CONST.SEARCH.ACTION_TYPES.SUBMIT]: 'common.submit', | ||
| [CONST.SEARCH.ACTION_TYPES.APPROVE]: 'iou.approve', | ||
| [CONST.SEARCH.ACTION_TYPES.PAY]: 'iou.pay', | ||
| [CONST.SEARCH.ACTION_TYPES.EXPORT_TO_ACCOUNTING]: 'common.export', | ||
| [CONST.SEARCH.ACTION_TYPES.DONE]: 'common.done', | ||
| [CONST.SEARCH.ACTION_TYPES.PAID]: 'iou.settledExpensify', | ||
| }; | ||
|
|
||
| export default actionTranslationsMap; |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ CONSISTENCY-5 (docs)
This
eslint-disable-next-linesuppresses@typescript-eslint/prefer-nullish-coalescingwithout an accompanying comment explaining why the logical OR (||) is preferred over nullish coalescing (??) here.Add a comment explaining the reason, for example:
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid comment (unless we specifically want to use
iouReport?.policyIDifpolicyIDis 0 (CONST.DEFAULT_NUMBER_ID)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH - this part of code was just moved - earlier it was like this in
src/components/SelectionListWithSections/Search/ActionCell.tsx#165.IMO adding a comment like a bit suggests seems bit redundant, as the code is self explanatory here. WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why was it implemented like that initially? Is there a reason we must use (
||) instead of (??)?This is the only instance throughout the app where it's used for policyID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, fair point I think it was introduced by this PR and I think there was no
@typescript-eslint/prefer-nullish-coalescingrule at that point.@getusha is there a reason you user
||here ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like an oversight to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure, but I think policyID can somehow be an empty string.