Repository navigation
Restore inline editing of the Expensify Card name - #103894
Conversation
…pencil The card name is a subtitle inside the cardholder cell, so EditableCell's padded box grew that line from 16px to 32px and opened a gap above the freeze message, which is why inline editing was removed from it as a temporary fix. EditableCell now takes shouldRenderDisplayChrome so a composite cell can keep its own line heights at rest, and InlineTextEditCell accepts that display block as children. The cardholder cell gets one pencil, and editing swaps the whole cell for the card name input, per the design agreed on the issue. Co-Authored-By: Claude Opus 5 <[email protected]>
The edit cell replaces its contents with the name input, so the avatar sits outside that cell.
|
Hold on #103678 |
|
CC @shawnborton Screen.Recording.2026-10-13.at.4.46.06.PM.mov |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Nice, that feels good to me! |
|
@MelvinBot Please update TestsPrecondition:
Offline testsSame as Tests step QA StepsSame as Tests step PR Author ChecklistMark all author checklist checked |
|
I updated the PR description:
I kept the "Verify that no errors appear in the JS console" checkbox under Tests and QA Steps. |
|
@MelvinBot Mark this PR ready for review Update: Sorry! I missed that I am the author of the PR. |
|
@Krishna2323 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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
This PR is already marked ready for review. It is open and not a draft, so I made no change. |
|
Sorry, Melvin! I missed that I’m the author of this PR. |
|
|
||
| /** | ||
| * 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 |
There was a problem hiding this comment.
❌ CONSISTENCY-15 (docs)
This newly added JSDoc comment for the children prop uses an em dash in its own sentence (value being edited — such as a cardholder block...). Comments should read as plain sentences without em dashes.
Rewrite the sentence without the em dash, for example:
/**
* 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;Reviewed at: 2d2b34e | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| * 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. | ||
| */ |
There was a problem hiding this comment.
❌ 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.
There was a problem hiding this comment.
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:
App/src/components/EditableCell/EditableCell.tsx
Lines 128 to 146 in ae7210c
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.
|
@MelvinBot please update those comments #103894 (comment) #103894 (comment) |
|
I can't push these changes because I'm not the author or an assignee on this PR. Here is what I would change for each comment. 1. Em dash in the /**
* 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;2.
Step 4 changes every existing |
trjExpensify
left a comment
There was a problem hiding this comment.
Nice to have it back! 👍
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMonosnap.screencast.2026-10-23.03-31-57.mp4 |
|
🚧 JS00001 has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.5.20-0 🚀
|
|
No help site update is needed. This PR restores inline editing of the card name, and Set-Up-and-Manage-the-Expensify-Card-US.md already says: "On the web, you can also hover over a card's Name, Limit type, or Limit in the list and click the pencil icon that appears to edit it inline." That sentence is still accurate. |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.5.20-5 🚀
Bundle Size Analysis (Sentry): |
The card name is a subtitle inside the cardholder cell, so EditableCell's padded box grew that line from 16px to 32px and opened a gap above the freeze message, which is why inline editing was removed from it as a temporary fix.
EditableCell now takes shouldRenderDisplayChrome so a composite cell can keep its own line heights at rest, and InlineTextEditCell accepts that display block as children. The cardholder cell gets one pencil, and editing swaps the whole cell for the card name input, per the design agreed on the issue.
Explanation of Change
Fixed Issues
$ #103150
PROPOSAL:
Tests
Precondition:
Offline tests
Same as Tests step
QA Steps
Same as Tests step
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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.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
Screen.Recording.2026-10-21.at.12.26.45.PM.mov
Screen.Recording.2026-10-21.at.12.28.30.PM.mov