Deleted transactions on search V2#86938
Conversation
…ode-persists-after-undeleting-fix
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
…sactions' into jnokowaw/deleted-transactions-on-search-v2
…nokowaw/deleted-transactions-on-search-v2
…-transactions-on-search-v2
…sactions-on-search-v2
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 040f009777
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| isInSingleTransactionReport={isInSingleTransactionReport} | ||
| shouldShowBottomBorder={shouldShowBottomBorder} | ||
| onArrowRightPress={() => openReportInRHP(transaction)} | ||
| onArrowRightPress={isDeletedTransaction(transaction) ? undefined : () => openReportInRHP(transaction)} |
There was a problem hiding this comment.
Block row navigation for deleted grouped transactions
This only disables the arrow callback for deleted child rows, but in narrow/grouped layouts the outer row press handler still calls handleOnPress(transaction) and opens the report/thread. As a result, tapping a deleted transaction row (outside the action button) still navigates instead of enforcing undelete-only behavior, which contradicts the new deleted transaction flow and can send users into an invalid detail path.
Useful? React with 👍 / 👎.
| disabled={isDisabled && !item.isSelected} | ||
| accessibilityLabel={item.text ?? ''} | ||
| role={getButtonRole(true)} | ||
| role={isDeletedTransaction ? getButtonRole(true) : 'none'} |
There was a problem hiding this comment.
Keep button role on clickable transaction rows
The role assignment is inverted: non-deleted rows remain clickable but are now rendered with role='none', while deleted rows get a button role even when onPress is disabled. This regresses accessibility semantics for the main transaction list (screen readers/keyboard users lose button semantics on active rows and may get misleading semantics on inactive ones).
Useful? React with 👍 / 👎.
|
@jnowakow how's this one coming along? Which bugs are left to be fixed? |
… into jnokowaw/deleted-transactions-on-search-v2
|
Thanks for the update! Let's keep the momentum on this one! |
|
I think that last thing is the case when user deletes all splits at once:
delete-all-splits-at-once.movWhat is expected behaviour in this situation? cc @lakchote @garrettmknight |
…-transactions-on-search-v2
|
I take my earlier comment back. This looks great. |
@luacmartins
Explanation of Change
This PR adds support deleted transactions on search.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/608639
PROPOSAL: N/A
Tests
Deletedstatus fileterOffline tests
QA Steps
Deletedstatus fileterPR 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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari