Refactor ConfirmModal usage to useConfirmModal in Search pages#77740
Refactor ConfirmModal usage to useConfirmModal in Search pages#77740roryabraham merged 10 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.
|
|
@ZhenjaHorbach 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] |
|
@roryabraham 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] |
|
The Android and IOS build is failing. Will update later. |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2025-12-18.10.34.25.mov2025-12-18.10.35.16.movAndroid: mWeb Chrome2025-12-18.10.42.17.mov2025-12-18.10.42.46.moviOS: HybridApp2025-12-18.10.28.37.mov2025-12-18.10.31.25.moviOS: mWeb Safari2025-12-18.10.23.09.mov2025-12-18.10.24.34.mov2025-12-18.10.25.54.movMacOS: Chrome / Safari2025-12-17.17.37.13.mov2025-12-17.17.37.44.mov2025-12-17.17.38.28.mov2025-12-18.10.06.07.mov |
Do you have this modal? |
|
You can see my PR's record. Screen.Recording.2025-12-06.at.23.51.55.mov |
|
Strange |
|
@ZhenjaHorbach I got this case. Fixed now. |
|
Okay But I hope @waterim will fix it soon |
|
@ZhenjaHorbach Was unable to fix it yet, hopefully tomorrow will be able to! I think better option here(at least for now) will be just to fix the modal closing and stick to navigation+closing to be able to merge all these PRs we have now. And leave the idea of closing modal without the navigation for the future not to introduce any hacks to the code and not to spend too much time on it. What do you think? (I have a lot on my plate now and only 3 days left before long vacations 😄) |
|
@nkdengineer |
|
@ZhenjaHorbach Updated. |
|
Noticed that we don't have animation after pressing 2026-01-06.12.00.19.mov |
|
@nkdengineer |
src/pages/Search/SearchPage.tsx
Outdated
| // Translations copy for delete modal depends on amount of selected items, | ||
| // We need to wait for modal to fully disappear before clearing them to avoid translation flicker between singular vs plural | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
| InteractionManager.runAfterInteractions(() => { |
There was a problem hiding this comment.
InteractionManager is deprecated, so we need to find another solution for this. The promise returned by showConfirmModal really should not resolve until the modal has fully disappeared.
There was a problem hiding this comment.
This code is from the previous implementation of the modal
But probably yes
Now this code seems redundant
There was a problem hiding this comment.
@nkdengineer bump on this comment - we need to address this before I can merge
There was a problem hiding this comment.
@roryabraham I just move the previous logic to here. Remove the Interaction can cause the regression.
There was a problem hiding this comment.
I see, you're right. I thought you had added a new usage, but it looks like you've just moved existing usages around. My mistake
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.0-0 🚀
|
@nkdengineer could you enable DEW for the account |
@nlemma I don't have a workspace with Maybe @ZhenjaHorbach can help |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.3.0-8 🚀
|
Explanation of Change
Fixed Issues
$ #76694
PROPOSAL:
Tests
SearchPage.tsx
Export in progress modal
Precondition: Has more than 50 expenses
Precondition: The approval mode of the workspace is
DYNAMIC_EXTERNALOffline tests
Same
QA Steps
Same as test
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
Screen.Recording.2025-12-16.at.16.13.24.mov
iOS: Native
Screen.Recording.2025-12-16.at.16.14.01.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2025-12-06.at.23.43.09.mov
Screen.Recording.2025-12-06.at.23.51.06.mov
Screen.Recording.2025-12-06.at.23.51.55.mov
Screen.Recording.2025-12-06.at.23.53.05.mov
Screen.Recording.2025-12-06.at.23.53.29.mov