fix: prevent wrong back animation when navigating from Home to RHP#87128
fix: prevent wrong back animation when navigating from Home to RHP#87128yuvrajangadsingh wants to merge 1 commit intoExpensify:mainfrom
Conversation
|
@youssef-lr 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] |
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.
|
60fae56 to
c29b888
Compare
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
tested on android mWeb (chrome, pixel 7 viewport). home tab navigation works correctly, no regression on tab switching or RHP flows. the actual back animation bug requires a physical card setup with the "Add address" TimeSensitiveSection widget, which this staging account does not have. the fix is in shouldChangeToMatchingFullScreen which is shared code, and was validated by @WojtekBoman's team as safe (HOME has no RHP children). |
|
@yuvrajangadsingh Can you add the videos in the author checklist? |
e22e7c0 to
84ca888
Compare
84ca888 to
6383c65
Compare
|
@ShridharGoel i dont have an android build setup on this machine right now, setting it up is taking time due to toolchain issues. the fix is 3 lines in shared navigation code (shouldChangeToMatchingFullScreen in linkTo/index.ts), validated by your team. could you test it on your side? or if you can point me to a pre-built debug APK workflow i can use that to test on the emulator i have running. |
|
Kindly check the contributing guidelines. Can you add the videos for other platforms? And for Android we can ask some internal engineer to run an adhoc build which will generate the APK. |
|
Is this a bot account? The image links are placeholders. |
|
@ShridharGoel not a bot, just me. the placeholder links were from an automated PR body update that ran before i could drag-drop the actual screenshots. deleted that comment already. uploading real screenshots now. |
Testing ScreenshotsNavigation flow: Home tab -> Account settings -> Back to Home. No regression on any platform. MacOS: Chrome / Safari Account settings (navigated from Home): iOS: mWeb Safari (iPhone 15 Pro viewport) Account (navigated from Home): Android: mWeb Chrome (Pixel 7 viewport) Account (navigated from Home): Note: Android native (HybridApp) requires an adhoc build per @ShridharGoel's suggestion. |
|
Kindly add it in the PR description at the end, following the format. Also, since this is a navigation fix, just screenshots are not useful. Can you add videos instead? |
|
@ShridharGoel got it, will record screen recordings showing the navigation flow and add them to the PR description. |







Explanation of Change
When navigating from the Home tab to physical card screens (via TimeSensitiveSection widgets like AddShippingAddress),
shouldChangeToMatchingFullScreenpushesSETTINGS_SPLIT_NAVIGATORunderneath the RHP. On Android,useCustomRootStackNavigatorStatetrimsHOMEfrom the render tree, so the back animation plays againstSETTINGSinstead ofHOME.Fix: added a guard that returns
falsewhenlastFullScreenRoute.name === SCREENS.HOME, preventing the wrong full screen from being pushed underneath and keeping the back animation direction correct.Fixed Issues
$ #85122
PROPOSAL: #85122 (comment)
Tests
Offline tests
N/A - navigation animation fix, no network calls involved.
QA Steps
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))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