Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ import {View} from 'react-native';

type TextWithIconCellProps = {
icon: IconAsset;
iconSize?: number;
text?: string;
showTooltip: boolean;
textStyle?: StyleProp<TextStyle>;
numberOfLines?: number;
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CONSISTENCY-2 (docs)

The new default parameter value iconSize = 12 is a magic number. The codebase already defines this exact value as a named constant, variables.iconSizeExtraSmall (which equals 12), and it is already used by other Search components (e.g. DropdownButton.tsx, WithdrawalIDListItemHeader.tsx). Using the raw literal here obscures its meaning and duplicates a value that lives in the style system.

Import variables and use the named constant as the default instead:

import variables from '@styles/variables';

export default function TextWithIconCell({icon, iconSize = variables.iconSizeExtraSmall, text, showTooltip, textStyle, numberOfLines = 1}: TextWithIconCellProps) {

Reviewed at: 62ea950 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

export default function TextWithIconCell({icon, text, showTooltip, textStyle, numberOfLines = 1}: TextWithIconCellProps) {
export default function TextWithIconCell({icon, iconSize = 12, text, showTooltip, textStyle, numberOfLines = 1}: TextWithIconCellProps) {
const styles = useThemeStyles();
const theme = useTheme();

Expand All @@ -32,8 +33,8 @@ export default function TextWithIconCell({icon, text, showTooltip, textStyle, nu
<Icon
src={icon}
fill={theme.icon}
height={12}
width={12}
height={iconSize}
width={iconSize}
/>
<TextWithTooltip
text={text}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import type {SearchTypeMenuItem, SearchTypeMenuSection} from '@libs/SearchUIUtil

import navigationRef from '@navigation/navigationRef';

import variables from '@styles/variables';

import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type {Route} from '@src/ROUTES';
Expand Down Expand Up @@ -149,8 +151,9 @@ function useNavigationSuggestions(query: string, shouldWatchForApprovals = true)
<TextWithIconCell
text={translate('common.spend')}
icon={icons.ReceiptMultiple}
iconSize={variables.fontSizeLabel}
showTooltip={false}
textStyle={styles.textLabelSupporting}
textStyle={[styles.textLabelSupporting, styles.label]}
/>
),
getItemText: (item) => translate(item.translationPath),
Expand Down
8 changes: 5 additions & 3 deletions tests/unit/SearchRouterNavigationTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import Navigation from '@libs/Navigation/Navigation';
import navigateToCannedSpendSearch from '@libs/SearchNavigationUtils';
import type {SearchTypeMenuItem, SearchTypeMenuSection} from '@libs/SearchUIUtils';

import variables from '@styles/variables';

import CONST from '@src/CONST';
import ROUTES from '@src/ROUTES';
import type IconAsset from '@src/types/utils/IconAsset';
Expand Down Expand Up @@ -416,11 +418,11 @@ describe('Spend Search Router navigation source', () => {
expect(result.current.some((item) => item.keyForList === `spend_${CONST.SEARCH.SAVED_SEARCH_PREFIX}1`)).toBe(false);

const rightElement = result.current.at(0)?.rightElement;
expect(isValidElement<{label: string; icon: IconAsset}>(rightElement)).toBe(true);
if (!isValidElement<{label: string; icon: IconAsset}>(rightElement)) {
expect(isValidElement<{text: string; icon: IconAsset; iconSize: number; showTooltip: boolean}>(rightElement)).toBe(true);
if (!isValidElement<{text: string; icon: IconAsset; iconSize: number; showTooltip: boolean}>(rightElement)) {
throw new Error('Expected Spend navigation context to be a React element');
}
expect(rightElement.props).toMatchObject({text: 'Spend', icon: spendContextIcon, showTooltip: false});
expect(rightElement.props).toMatchObject({text: 'Spend', icon: spendContextIcon, iconSize: variables.fontSizeLabel, showTooltip: false});

rerender({shouldWatchForApprovals: true});
expect(mockUseSearchTypeMenuSections).toHaveBeenLastCalledWith(undefined, true);
Expand Down
Loading