fix: remove flashing effect when opening a report#56523
fix: remove flashing effect when opening a report#56523youssef-lr merged 8 commits intoExpensify:mainfrom
Conversation
|
@mananjadhav 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] |
|
@youssef-lr @puneetlath Could you help with the adhoc build here? and probably a test on your account @puneetlath would also be helpful. |
|
Triggered the build. |
|
🚧 @puneetlath has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Thanks, would you be able to check the adhoc build against your account @puneetlath ? |
|
Here's what it looks like on staging: Here's what it looks like on adhoc: So it seems a little bit better, though still slightly jumpy. |
|
@daledah Can you please check the previous comment by @puneetlath . |
|
@mananjadhav @puneetlath I updated using the alternate solution, and the issue doesn't exist anymore on my end. Screen.Recording.2025-02-20.at.23.47.07.mov |
|
Can we generate an adhoc build @puneetlath ? Checking the code at my end too. |
src/hooks/useCurrentReportID.tsx
Outdated
| (): CurrentReportIDContextValue => ({ | ||
| updateCurrentReportID, | ||
| currentReportID, | ||
| currentReportID: lastAccessReportFromPath ?? currentReportID, |
There was a problem hiding this comment.
I don't understand this change. Can you elaborate on how your solution works?
There was a problem hiding this comment.
lastAccessReportFromPath updates value faster than currentReportID, so using it here will prevent currentReportID context value from being outdated.
|
🚧 @puneetlath has triggered a test hybrid app build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
🚧 @puneetlath has triggered a test build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
This seems to work quite well for me now. It no longer has the flashing. |
|
Thanks for the feedback, I'll have this tested at my end too for other usages of useCurrentReportID. |
|
@daledah I am checking |
Test case:
Screen.Recording.2025-02-28.at.16.01.41.mov |
|
@mananjadhav I updated the logics to only apply the changes to current bug to avoid regression on other places. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-chat-switch-flicker.movAndroid: mWeb Chromemweb-chrome-chat-switch-flicker.moviOS: Nativeios-chat-switch-flicker.moviOS: mWeb Safarimweb-safari-chat-switch-flicker.movMacOS: Chrome / Safariweb-chat-switch-flicker-3.movweb-chat-switch-flicker-2.movweb-chat-switch-flicker-1.movMacOS: Desktopdesktop-chat-switch-flicker.mov |
|
✋ 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/youssef-lr in version: 9.1.9-0 🚀
|
|
🚀 Deployed to staging by https://github.com/youssef-lr in version: 9.1.9-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.1.9-8 🚀
|
| const {accountID} = useCurrentUserPersonalDetails(); | ||
| const currentReportIDValue = useCurrentReportID(); | ||
| const derivedCurrentReportID = currentReportIDForTests ?? currentReportIDValue?.currentReportID; | ||
| const derivedCurrentReportID = currentReportIDForTests ?? currentReportIDValue?.currentReportIDFromPath ?? currentReportIDValue?.currentReportID; |
There was a problem hiding this comment.
We removed currentReportIDFromPath in #73457 as the flicker or flashing is not observed anymore.
Explanation of Change
Fixed Issues
$ #55691
PROPOSAL: #55691 (comment)
Tests
Precondition: Enable focus mode
Offline tests
QA Steps
Precondition: Enable focus mode
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))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
Screen.Recording.2025-02-07.at.13.37.55.mov
Android: mWeb Chrome
Screen.Recording.2025-02-07.at.13.38.51.mov
iOS: Native
Screen.Recording.2025-02-07.at.13.39.21.mov
iOS: mWeb Safari
Screen.Recording.2025-02-07.at.13.50.52.mov
MacOS: Chrome / Safari
Screen.Recording.2025-02-07.at.13.42.46.mov
MacOS: Desktop
Screen.Recording.2025-02-07.at.13.51.10.mov