feat: Add inline editing for tables on desktop (V2)#89166
feat: Add inline editing for tables on desktop (V2)#89166mohammadjafarinejad wants to merge 44 commits intoExpensify:mainfrom
Conversation
This reverts commit 78be79e. Co-authored-by: Copilot <copilot@github.com>
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
|
@mohammadjafarinejad when can you have this ready for review? |
|
And let us know when we can adhoc test build 😄 |
|
Bump @mohammadjafarinejad. Can we get daily updates please to get this back out? |
|
Bump again @mohammadjafarinejad. Please provide an update. |
|
@trjExpensify I'll have it ready for review by tomorrow, after I test it to make sure all bugs are fixed. |
|
Okay, thank you. |
| allowFlippingAmount | ||
| isNegative={isNegative} |
There was a problem hiding this comment.
Let’s follow this to avoid allowing fields that don’t support negative amounts.
| if (!policy?.areCategoriesEnabled) { | ||
| return false; |
There was a problem hiding this comment.
I think we should also check if the category is missing, so we can remove the selected category even if the category feature is disabled.
if (!policy?.areCategoriesEnabled && isCategoryMissing(transaction?.category)) {
Screen.Recording.2026-05-05.at.2.43.53.AM.mov
| forwardedFSClass = CONST.FULLSTORY.CLASS.UNMASK, | ||
| ref, | ||
| shouldDisplayBelowModals = false, | ||
| shouldEnableBackdropInNarrowPane = false, |
There was a problem hiding this comment.
shouldEnableBackdropInNarrowPane seems a bit generic. Since this behavior is specific to right-docked modals rendered in narrow layouts, I suggest renaming it to shouldKeepRightDockedBackdropInNarrowPane for better clarity and self-documentation at call sites.
I believe this change is only needed for the BaseModal.tsx component, since it’s a global component, and isn’t needed for the YearPickerModal.tsx and MonthPickerModal.tsx components.
| return ( | ||
| <PressableWithFeedback | ||
| accessibilityRole={CONST.ROLE.BUTTON} | ||
| accessibilityLabel="Edit cell" |
There was a problem hiding this comment.
I believe we should discuss/address accessibility in the follow-up because the search table has many expense rows, and each row has multiple input types.
If we need to support it on the table (this accessibility action should be used to focus the cell and start editing), we should have options like “Edit amount” and “Edit Tag.” Additionally, we might need to create a separate “Edit amount” option for transaction X.
| return; | ||
| } | ||
| hasEndedRef.current = true; | ||
| onSave?.(localValue); |
There was a problem hiding this comment.
Should we return early here if the value hasn’t changed to avoid redundant API calls?
if (localValue === value) {
setIsEditing(false);
return;
}
There was a problem hiding this comment.
@mohammadjafarinejad We have an edge case with the above suggestion when flipping between negative and positive amounts. The - symbol lives outside the input, so switching to a negative value is not considered a change.
Passing a new allowSavingUnchangedValue param to useInlineEditState for the amount cell fixes this issue.
if (!allowSavingUnchangedValue && localValue === value) {
setIsEditing(false);
return;
}
There was a problem hiding this comment.
@ahmedGaber93 We normalize values in MerchantCell and TotalCell before saving, so a plain equality check is not enough. I added an optional equality function to useInlineEditState so each field can compare normalized values.
const getNormalizedValue = (value: string) => (isDescription ? value : value.trim());
const {isEditing, localValue, setLocalValue, startEditing, save, cancelEditing} = useInlineEditState(
canEdit,
text,
(value) => onSave?.(getNormalizedValue(value)),
(value, originalValue) => getNormalizedValue(value) === getNormalizedValue(originalValue),
);WDYT about this approach?
| // Unreported transactions can have empty merchants (allows clearing) | ||
| const isUnreported = transaction ? isExpenseUnreported(transaction) : false; | ||
| if (isEmpty && isUnreported) { | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
I’m not sure why the suggestion above doesn’t work for me when I test it now. If it doesn’t work for you either, we can use this alternative instead.
const isIOU = isIOUReport(transaction?.reportID);
if (isEmpty && (isUnreported || isIOU)) {
| const [isInverted, setIsInverted] = useState(false); | ||
| const [adjustedPopoverHeight, setAdjustedPopoverHeight] = useState(popoverHeight); | ||
|
|
||
| const openPopover = () => { |
There was a problem hiding this comment.
I think the previous version looks more stable. We now have dynamic height, and the issue is still reproducible.
I think it would be better if we keep the previous version and handle this in a follow-up to keep this PR focused.
Current
Screen.Recording.2026-05-05.at.11.37.57.AM.mov
previous version
Screen.Recording.2026-05-05.at.11.28.44.AM.mov
| editContent={ | ||
| <TextInput | ||
| ref={handleRef} | ||
| accessibilityLabel={isDescription ? 'Description input' : 'Merchant input'} |
There was a problem hiding this comment.
Could we localize this accessibility label instead of hardcoding English text?
|
@mohammadjafarinejad, I’ve completed my review. Please let me know when it’s ready for review again. Thanks! |
@dubielzyk-expensify @Expensify/design It is ready for review. We manage to fix those on follow-up if they not blockers #88750 → Likely due to missing Onyx data, needs deeper investigation Inline edit for description on multiline |
|
@puneetlath did you want to review this one too? |
|
Yes! I'll take a look once @ahmedGaber93's feedback has been addressed. |
@mohammadjafarinejad This looks not related to our PR, Amount field is hidden even when it is selected in the column configuration. Commenting on the offending PR https://github.com/Expensify/App/pull/86824/changes#r3191023822 that causes the issue since it still on regression period If you want to force display it for testing, set this value to false and confirm Amount is selected in the columns configuration. Line 5218 in b0a9339 Screen.Recording.2026-05-05.at.10.50.23.PM.mov |
|
We have a new update here: #86824 (comment) Now, we need to have at least one expense with a foreign currency to display the This may cause a UX conflict for inline edit:
@trjExpensify WDYT? Should we exchange them instead?
|
This would be contrary to Classic though? Total (convertedAmount) is always visible, Amount is shown when there's foreign expenses. |
|
@trjExpensify Ah, I think we have some options for that.
|
|
@ahmedGaber93 Ready for re-review. |
|
@ahmedGaber93 for now I think we should allow the
I agree this could be a possible alternative, but it's implicit logic that isn't super obvious, so I think let's reserve it for a follow-up if necessary. |
ahmedGaber93
left a comment
There was a problem hiding this comment.
Just two comments, then we can move forward
| useEffect(() => { | ||
| if (isVisible) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is this change related to our PR? If not, I think we can revert it to avoid unexpected regression
| hash, | ||
| transactionID, | ||
| parentReport: parentReport ?? fallbackReport, | ||
| transactionThreadReport: transactionThreadReport ?? parentReport ?? fallbackReport, |
There was a problem hiding this comment.
@mohammadjafarinejad Passing parentReport as a fallback here is causing an issue for me. When editing a cell from Spend > Expenses, a system message is added to both the expense report and the transaction report.
I think we should avoid falling back to parentReport here.
const transactionThreadReportFallback: OnyxEntry<Report> = transactionThreadReportID ? ({reportID: transactionThreadReportID} as Report) : undefined;and pass it into getEditParams and getTransactionEditPermissions
transactionThreadReport: transactionThreadReport ?? transactionThreadReportFallback,Screen.Recording.2026-05-07.at.1.31.13.AM.mov
There was a problem hiding this comment.
@mohammadjafarinejad I removed the suggested fix on the comment above. Although it works since the correct reportID is being passed, the missing attributes on the report could lead to unexpected issues and it feels like a fragile solution.
What cases cause this report to be missing? And what would happen if we don’t provide a fallback at all?
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-05-07.at.4.03.45.AM.movAndroid: mWeb ChromeScreen.Recording.2026-05-07.at.4.05.48.AM.moviOS: HybridAppScreen.Recording.2026-05-07.at.3.57.14.AM.moviOS: mWeb SafariScreen.Recording.2026-05-07.at.4.03.08.AM.movMacOS: Chrome / SafariScreen.Recording.2026-05-07.at.1.44.13.AM.mov |
|
This Issue already exists on staging, and not related to our PR Screen.Recording.2026-05-07.at.1.44.13.AM.mov |
|
@ahmedGaber93 are you done with your review? Thanks! |
|
Yes, @mohammadjafarinejad just the above 2 comment #89166 (review) |
This reverts commit 78be79e.
Explanation of Change
This PR reapplies #83127 (inline editing for tables on desktop), which was reverted in #88751, with the original issues fixed and additional improvements.
Fixed Issues
$ #82534
$ #88711
PROPOSAL: #82534 (comment)
Tests
Offline tests
QA Steps
Same as tests.
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.