Skip to content
23 changes: 20 additions & 3 deletions src/components/EditableCell/EditableCell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ type EditableCellProps = {

/** Which side should show the edit icon in display mode */
editIconPosition?: EditIconPosition;

/**
* Whether display mode wraps children in the padded, bordered `editableCell` box.
* A composite cell whose display content is taller than one line owns its own layout,
* and the box would add height to it. Edit mode still draws the box around its input.
*/

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.

❌ CLEAN-REACT-PATTERNS-1 (docs)

The new shouldRenderDisplayChrome boolean prop is a configuration flag that toggles a layout strategy inside EditableCell (whether to wrap children in the padded, bordered editableCell box). It drives ternary branching at two render sites (shouldRenderDisplayChrome ? <View style={styles.editableCell}>{children}</View> : children), so the component has to grow a new flag and new internal branches to support a composite cell that owns its own layout. This is configuration-over-composition: a prop that selects a rendering strategy is a behavioral flag, not a presentational one.

Prefer composition: let the consumer decide whether the box is drawn rather than threading a flag through EditableCell. For example, expose the display chrome as a composable wrapper (e.g. a <EditableCell.DisplayBox> sub-component the caller includes or omits) so a composite cell can pass its own content without the box, and EditableCell stays stable instead of branching on shouldRenderDisplayChrome.


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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Turning that flag into a DisplayBox child changes the layout of the other editable cells. so it will require many changes on all onther cells, so I left shouldRenderDisplayChrome in place.

The flag only applies while the cell is showing its display content. Edit mode and popover mode always draw the editableCell box themselves:

// Inline edit mode: replace display content with the edit input
if (isEditing && editContent) {
return <View style={[styles.editableCell, styles.editableCellFocus]}>{editContent}</View>;
}
// Popover edit mode: keep showing display content with active border
if (isEditing && popoverContent) {
return (
<>
<View
ref={anchorRef}
style={[styles.editableCell, styles.editableCellFocus]}
>
{children}
</View>
{popoverContent}
</>
);
}

That box is padding, a transparent border, and a minimum height. Date, category, tag, and the other popover cells rely on EditableCell to add it. If the caller also wrapped the display content in DisplayBox, opening the picker would nest a second box: extra padding and a taller cell. If EditableCell stopped drawing the box and every caller added DisplayBox, any missed caller would lose that padding, and the popover focus border would still have to be applied from inside EditableCell.

The card-name cell is the one caller that opts out, because its display content is the cardholder block and the box would add height. Editing that cell still needs the box around the input. A child the caller includes or omits cannot describe both of those states.

shouldRenderDisplayChrome?: boolean;
};

/**
Expand All @@ -61,7 +68,17 @@ type EditableCellProps = {
* 4. canEdit=false → styled container View, no pressable (transient: loading / no permission)
* 5. default → passive cell body with hover/focus edit icon button
*/
function EditableCell({children, editContent, popoverContent, isEditing, canEdit, onStartEditing, anchorRef, editIconPosition = 'right'}: EditableCellProps) {
function EditableCell({
children,
editContent,
popoverContent,
isEditing,
canEdit,
onStartEditing,
anchorRef,
editIconPosition = 'right',
shouldRenderDisplayChrome = true,
}: EditableCellProps) {
const styles = useThemeStyles();
const theme = useTheme();
const {translate} = useLocalize();
Expand Down Expand Up @@ -133,7 +150,7 @@ function EditableCell({children, editContent, popoverContent, isEditing, canEdit
// Tooltip / Hoverable / BoundsObserver subtree during the initial skeleton render,
// unblocking the main thread so the network response can be processed sooner.
if (!canEdit || !isInteractive) {
return <View style={[styles.editableCell]}>{children}</View>;
return shouldRenderDisplayChrome ? <View style={[styles.editableCell]}>{children}</View> : children;
}

return (
Expand All @@ -143,7 +160,7 @@ function EditableCell({children, editContent, popoverContent, isEditing, canEdit

return (
<View style={[styles.flex1, styles.w100, styles.justifyContentCenter]}>
<View style={styles.editableCell}>{children}</View>
{shouldRenderDisplayChrome ? <View style={styles.editableCell}>{children}</View> : children}
<View
pointerEvents={shouldShowEditIcon ? 'box-none' : 'none'}
style={[
Expand Down
28 changes: 20 additions & 8 deletions src/components/EditableCell/InlineTextEditCell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import StringUtils from '@libs/StringUtils';

import CONST from '@src/CONST';

import type {ReactNode} from 'react';
import type {StyleProp, TextStyle} from 'react-native';

import React, {useRef} from 'react';
Expand All @@ -24,14 +25,22 @@ type InlineTextEditCellProps = {

accessibilityLabel: string;

/** Style applied to the display text and to the TextInput while editing, so font size and color stay in sync */
/** Style for the default display text and the TextInput while editing. With children, only the input uses it. */
displayTextStyle?: StyleProp<TextStyle>;

/**
* Normalizes the value before saving and for change detection.
* Defaults to sanitizeName so a pasted non-breaking space is treated as unchanged when the action would no-op.
*/
sanitize?: (value: string) => string;

/**
* Display content replacing the default single-line text, for a cell that shows more than the
* value being edited, such as a cardholder block whose subtitle is the card name. The box that
* normally wraps display content is dropped so that layout keeps its own line heights. Editing
* still swaps the whole cell for the input.
*/
children?: ReactNode;
Comment thread
ahmedGaber93 marked this conversation as resolved.
Resolved
} & EditableProps<string>;

/**
Expand All @@ -43,7 +52,7 @@ type InlineTextEditCellProps = {
* original value. On narrow layouts EditableCell renders the display text only, so tables keep
* their existing tap-to-navigate behavior there.
*/
function InlineTextEditCell({value, accessibilityLabel, displayTextStyle, canEdit, onSave, sanitize = StringUtils.sanitizeName}: InlineTextEditCellProps) {
function InlineTextEditCell({value, accessibilityLabel, displayTextStyle, canEdit, onSave, sanitize = StringUtils.sanitizeName, children}: InlineTextEditCellProps) {
const styles = useThemeStyles();
const inputRef = useRef<BaseTextInputRef | null>(null);

Expand All @@ -68,6 +77,7 @@ function InlineTextEditCell({value, accessibilityLabel, displayTextStyle, canEdi
canEdit={canEdit}
isEditing={isEditing}
onStartEditing={startEditing}
shouldRenderDisplayChrome={!children}
editContent={
<TextInput
ref={inputRef}
Expand All @@ -87,12 +97,14 @@ function InlineTextEditCell({value, accessibilityLabel, displayTextStyle, canEdi
/>
}
>
<TextWithTooltip
shouldShowTooltip
text={localValue}
numberOfLines={1}
style={displayTextStyle}
/>
{children ?? (
<TextWithTooltip
shouldShowTooltip
text={localValue}
numberOfLines={1}
style={displayTextStyle}
/>
)}
</EditableCell>
);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import UserAvatar from '@components/Avatar/UserAvatar';
import {InlineTextEditCell} from '@components/EditableCell';
import Icon from '@components/Icon';
import {useSession} from '@components/OnyxListItemProvider';
import Table from '@components/Table';
Expand Down Expand Up @@ -126,33 +127,44 @@ export default function WorkspaceExpensifyCardsTableRow({item, rowIndex, shouldU
style={[styles.flex1, styles.mnw0, styles.flexRow, styles.gap3, styles.alignItemsCenter]}
{...getCellAccessibilityProps(isTableSemanticsEnabled)}
>
{/* The avatar sits outside the edit cell so it stays on screen while the card name is edited. */}
<UserAvatar
source={item.cardholder?.avatar}
accountID={item.cardholder?.accountID ?? CONST.DEFAULT_NUMBER_ID}
size={avatarSize}
/>
<View style={[styles.flex1, styles.mnw0, shouldUseNarrowTableLayout && styles.gap1]}>
<TextWithTooltip
shouldShowTooltip
numberOfLines={1}
text={cardholderName}
style={[styles.optionDisplayName, styles.pre]}
/>
{shouldUseNarrowTableLayout ? (
<TextWithTooltip
shouldShowTooltip
numberOfLines={1}
text={narrowLayoutSubtitle}
style={[styles.textLabelSupporting, styles.lh16, styles.pre, styles.mr3]}
/>
) : (
<TextWithTooltip
shouldShowTooltip
numberOfLines={1}
text={item.name}
style={styles.textLabelSupporting}
/>
)}
<View style={[styles.flex1, styles.mnw0]}>
<InlineTextEditCell
value={item.name}
accessibilityLabel={translate('workspace.card.issueNewCard.cardName')}
canEdit={!!item.canEditName}
onSave={item.onRenameName}
displayTextStyle={[styles.textLabelSupporting, styles.lh16]}
>
<View style={[styles.mnw0, shouldUseNarrowTableLayout && styles.gap1]}>
<TextWithTooltip
shouldShowTooltip
numberOfLines={1}
text={cardholderName}
style={[styles.optionDisplayName, styles.pre]}
/>
{shouldUseNarrowTableLayout ? (
<TextWithTooltip
shouldShowTooltip
numberOfLines={1}
text={narrowLayoutSubtitle}
style={[styles.textLabelSupporting, styles.lh16, styles.pre, styles.mr3]}
/>
) : (
<TextWithTooltip
shouldShowTooltip
numberOfLines={1}
text={item.name}
style={styles.textLabelSupporting}
/>
)}
</View>
</InlineTextEditCell>
</View>
</View>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,11 @@ type WorkspaceExpensifyCardTableRowData = TableData & {
frozenDate?: string;
errors?: OnyxCommon.Errors;
pendingAction?: OnyxCommon.PendingAction;
canEditName?: boolean;
canEditLimitType?: boolean;
canEditLimit?: boolean;
action: () => void;
onRenameName?: (newName: string) => void;
onChangeLimitType?: (limitType: CardLimitType) => void;
/** Return false, or a promise of false, to keep the limit editor open until a confirm modal resolves. */
onChangeLimit?: (newLimit: string) => InlineEditSaveResult;
Expand Down
18 changes: 17 additions & 1 deletion src/libs/actions/Policy/InlineEdit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {getCleanedTagName, getTagList} from '@libs/PolicyUtils';
import StringUtils from '@libs/StringUtils';
import {getTagNameError, getTagNameErrorMessage} from '@libs/TagUtils';

import {updateExpensifyCardLimit, updateExpensifyCardLimitType} from '@userActions/Card';
import {updateExpensifyCardLimit, updateExpensifyCardLimitType, updateExpensifyCardTitle} from '@userActions/Card';
import {updateCompanyCardName} from '@userActions/CompanyCards';

import CONST from '@src/CONST';
Expand Down Expand Up @@ -103,6 +103,21 @@ function renameCompanyCardInline(
updateCompanyCardName(domainOrWorkspaceAccountID, cardID, sanitized, bankName, storedName);
}

function renameExpensifyCardInline(translate: LocalizedTranslate, workspaceAccountID: number, cardID: number, newName: string, currentName: string): string | undefined {
const sanitized = StringUtils.sanitizeName(newName);

if (sanitized === currentName) {
return;
}

const cardNameError = getCardNameError(newName);
if (cardNameError) {
return getCardNameErrorMessage(translate, cardNameError, newName);
}

updateExpensifyCardTitle(workspaceAccountID, cardID, sanitized, currentName);
}

function renameDistanceRateInline(translate: LocalizedTranslate, policyID: string, customUnit: CustomUnit, rate: Rate, newName: string): string | undefined {
const sanitized = StringUtils.sanitizeName(newName);
const currentName = rate.name ?? '';
Expand Down Expand Up @@ -300,6 +315,7 @@ export {
renameCategoryInline,
renameTagInline,
renameCompanyCardInline,
renameExpensifyCardInline,
renameDistanceRateInline,
updateDistanceRateValueInline,
updateMemberRoleInline,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
canUpdateExpensifyCardLimitTypeInline,
getExpensifyCardLimitInlineEditError,
getExpensifyCardLimitInlineUpdate,
renameExpensifyCardInline,
updateExpensifyCardLimitInline,
updateExpensifyCardLimitTypeInline,
} from '@libs/actions/Policy/InlineEdit';
Expand Down Expand Up @@ -276,9 +277,11 @@ function WorkspaceExpensifyCardListPage({route, cardsList, fundID}: WorkspaceExp
frozenDate: card.nameValuePairs?.frozen?.date,
errors: card.errors,
pendingAction: card.pendingAction,
canEditName: canEditCard,
canEditLimitType: canEditCard,
canEditLimit: canEditCard,
action: () => Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.WORKSPACE_EXPENSIFY_CARD_DETAILS.getRoute(card.cardID.toString()))),
onRenameName: (newName: string) => showInlineEditError(renameExpensifyCardInline(translate, fundID, card.cardID, newName, card.nameValuePairs?.cardTitle ?? '')),
onChangeLimitType: (newLimitType: CardLimitType) => changeCardLimitType(card, newLimitType),
onChangeLimit: (newLimit: string) => changeCardLimit(card, newLimit),
onClose: () => clearDeletePaymentMethodError(`${ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST}${fundID}_${CONST.EXPENSIFY_CARD.BANK}`, card.cardID),
Expand Down
62 changes: 62 additions & 0 deletions tests/unit/inlineEditing/PolicyInlineEdit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
getExpensifyCardLimitInlineEditError,
getExpensifyCardLimitInlineUpdate,
renameCategoryInline,
renameExpensifyCardInline,
updateExpensifyCardLimitInline,
updateExpensifyCardLimitTypeInline,
updateMemberRoleInline,
Expand Down Expand Up @@ -177,6 +178,67 @@ describe('PolicyInlineEdit', () => {
});
});

describe('renameExpensifyCardInline', () => {
it('does not persist a name the user never changed', () => {
// Given a card named "Travel"
// When the editor closes on blur with that same name
const rejection = renameExpensifyCardInline(translate, 1, 10, 'Travel', 'Travel');

// Then nothing is written and no toast is raised, so opening the editor and clicking away costs no request
expect(rejection).toBeUndefined();
expect(mockWrite).not.toHaveBeenCalled();
});

it('returns the required-field error for a name that sanitizes to nothing', () => {
// Given a card named "Travel"
// When the editor commits whitespace only
const rejection = renameExpensifyCardInline(translate, 1, 10, ' ', 'Travel');

// Then the page can toast the same required message as the card name form, and nothing is written
expect(rejection).toBe('common.error.fieldRequired');
expect(mockWrite).not.toHaveBeenCalled();
});

it('returns the invalid-character error for a name the card form rejects', () => {
// Given a card named "Travel"
// When the editor commits a name that contains an HTML-like tag
const rejection = renameExpensifyCardInline(translate, 1, 10, '</>', 'Travel');

// Then the page can toast the same invalid-character message as the card name form, and nothing is written
expect(rejection).toBe('common.error.invalidCharacter');
expect(mockWrite).not.toHaveBeenCalled();
});

it('returns the length error for a name past the card form limit', () => {
// Given a card named "Travel" and a name one byte past the shared length limit
const tooLongName = 'a'.repeat(CONST.STANDARD_LENGTH_LIMIT + 1);

// When the editor commits that name
const rejection = renameExpensifyCardInline(translate, 1, 10, tooLongName, 'Travel');

// Then the page can toast the same character-limit message as the card name form, and nothing is written
expect(rejection).toBe(`common.error.characterLimitExceedCounter|${CONST.STANDARD_LENGTH_LIMIT + 1}|${CONST.STANDARD_LENGTH_LIMIT}`);
expect(mockWrite).not.toHaveBeenCalled();
});

it('persists the sanitized name', () => {
// Given a card named "Travel"
// When the editor commits a new name padded with whitespace
const rejection = renameExpensifyCardInline(translate, 1, 10, ' Travel card ', 'Travel');

// Then the trimmed name reaches the backend and no toast is raised, so a padded paste cannot be stored verbatim
expect(rejection).toBeUndefined();
expect(mockWrite).toHaveBeenCalledWith(
WRITE_COMMANDS.UPDATE_EXPENSIFY_CARD_TITLE,
expect.objectContaining({
cardID: 10,
cardTitle: 'Travel card',
Comment thread
ahmedGaber93 marked this conversation as resolved.
Resolved
}),
expect.anything(),
);
});
});

describe('getExpensifyCardLimitInlineUpdate', () => {
it('returns the field error for an invalid limit so the page can toast it', () => {
// Given a limit the card form rejects because it is not a whole dollar amount
Expand Down
Loading