Fix - Unable to hold expenses from dropdown menu#75833
Fix - Unable to hold expenses from dropdown menu#75833Tony-MK wants to merge 2 commits intoExpensify:mainfrom
Conversation
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.
|
|
@dominictb 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] |
| context.clearSelectedTransactions(); | ||
| } | ||
|
|
||
| Navigation.goBack(); |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing the entire context object as a dependency causes the useCallback hook to re-execute whenever any property of context changes, even if those properties are not used in the callback. This reduces performance predictability and can lead to unnecessary re-executions.
Suggested fix: Specify individual context properties as dependencies:
const onSubmit = useCallback(
({comment}: FormOnyxValues<typeof ONYXKEYS.FORMS.MONEY_REQUEST_HOLD_FORM>) => {
if (route.name === SCREENS.SEARCH.MONEY_REQUEST_REPORT_HOLD_TRANSACTIONS) {
putTransactionsOnHold(context.selectedTransactionIDs, comment, reportID, ancestors);
context.clearSelectedTransactions(true);
} else {
holdMoneyRequestOnSearch(context.currentSearchHash, Object.keys(context.selectedTransactions), comment, allTransactions, allReportActions);
context.clearSelectedTransactions();
}
Navigation.goBack();
},
[route.name, context.selectedTransactionIDs, context.clearSelectedTransactions, context.currentSearchHash, context.selectedTransactions, reportID, allTransactions, allReportActions, ancestors],
);|
Bump @situchan. |
|
Please add screen recordings on all platforms |
trjExpensify
left a comment
There was a problem hiding this comment.
Looks like this was fixing a blocker that was reverted, but we'll still want to fix this on the original issue. 👍
|
Bump @Tony-MK |
|
@Tony-MK what's the next step here? |
|
@Tony-MK we have conflicts. |
|
@Tony-MK let me know if you're not available to work on this and we should reassign the issue. |
|
There has been no movement here for over a month. I'm closing this PR and reassigned the issue to a new contributor. |
|
Apologies @luacmartins, I wasn't feel well. I should have informed you. However, I believe it was a good decision to assign another contributor. |
Explanation of Change
Adding fix to fix blocker: #75788
Fixed Issues
$ #75788
PROPOSAL:
Tests
Prerequisite: The Account has at least one workspace.
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
MacOS: Desktop
macOS.-.Desktop.mov