fix resolve delay when dismissing attachment preview#53108
fix resolve delay when dismissing attachment preview#53108Beamanator merged 8 commits intoExpensify:mainfrom
Conversation
|
@thesahindia 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] |
|
@thesahindia @Beamanator I tested the Report Avatar, Profile Avatar, and Workspace Avatar, and they have the same issue. Therefore, I added a fix for them to this pull request |
…t-preview-dismiss-delay
|
@thesahindia ESLint has failed on the main branch. This might have happened due to the Onyx migration. I noticed that withOnyx was removed. |
|
There's a minor issue. After closing the attachment preview, you need to wait briefly before you can open it again. Before the changes: Screen.Recording.2024-11-28.at.1.14.24.AM.movAfter the changes: Screen.Recording.2024-11-28.at.1.16.27.AM.mov |
This comment was marked as outdated.
This comment was marked as outdated.
|
@thesahindia I've updated this pull request to resolve your issue. Could you please check it again? Thank you. |
|
On iOS, the attachment modal takes some time to appear Screen.Recording.2024-11-29.at.12.06.39.AM.mov |
|
@thesahindia Yes, that is the behavior of the main branch, and this pull request does not impact it. We are only handling the behavior when onClose is triggered. Screen.Recording.2024-11-29.at.10.11.42.mp4 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-12-03.at.1.22.13.AM.movAndroid: mWeb ChromeScreen.Recording.2024-12-03.at.1.24.09.AM.moviOS: Nativehttps://github.com/user-attachments/assets/7fc90470-7d6f-430f-be9a-bb4eaaf3c4bbiOS: mWeb SafariScreen.Recording.2024-11-30.at.2.57.24.AM.movMacOS: Chrome / SafariScreen.Recording.2024-11-28.at.1.14.24.AM.movMacOS: DesktopScreen.Recording.2024-11-30.at.3.00.53.AM.mov |
What do y'all plan to do about this eslint error? |
|
Ahh yes! Forgot to comment about that. @huult, could you please update |
@thesahindia I've replaced withOnyx with useOnyx. Could you check it again? |
|
One more question: Wouldn't it be better to use the new |
|
@Beamanator I think it's good. I'll update it now. |
|
@Beamanator @thesahindia I have updated the pull request. Could you please check it? |
|
I'm loving the simplification! Still just looking for that comment in the code plz 🙏 |
|
@Beamanator Yes, I’m writing it now |
|
@Beamanator I’ve added a description to explain why we need InteractionManager.runAfterInteractions. Can you check it? If you have any suggestions for me, let me know. Thank you! |
|
The changes looks fine. I will test the flow again with the new changes in the morning. |
Beamanator
left a comment
There was a problem hiding this comment.
Code & comment look great! Will wait for full retest before merging 🙏
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.0.73-0 🚀
|
|
Found KI when validation the PR - issue #53644 |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.0.73-8 🚀
|
Details
Fixed Issues
$ #52937
PROPOSAL: #52937 (comment)
Tests
Same QA step
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Screen.Recording.2024-11-26.at.10.01.50.mp4
Android: mWeb Chrome
Screen.Recording.2024-11-26.at.09.53.51.mp4
iOS: Native
Screen.Recording.2024-11-26.at.09.41.52.mp4
iOS: mWeb Safari
Screen.Recording.2024-11-26.at.09.43.44.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-11-26.at.09.55.10.mp4
MacOS: Desktop
Screen.Recording.2024-11-26.at.10.03.14.mp4