-
Notifications
You must be signed in to change notification settings - Fork 3.5k
chore: Upgrade react-navigation to latest #76933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Upgrade react-navigation to latest #76933
Conversation
|
@aimane-chnaif 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✅ Changes either increased or maintained existing code coverage, great job!
|
d0833c2 to
00742ac
Compare
00742ac to
0cac09a
Compare
|
Removing my review |
aimane-chnaif
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patches/react-navigation/@react-navigation+core+7.10.0+002+fix-crash-when-parsing-emoji.patch
Is this patch not needed anymore?
|
@blazejkustra sorry for the ping but we'd appreciate your review on TS side |
Sure thing 😄 Btw shouldn't there be a PR updating pods in hybrid app too? Does @TaduJR have access to OldDot? |
|
Also how is this PR 'No QA'? This is a minor upgrade, yes. But still, I would be afraid to just push it to main without QA involvement 😅 |
blazejkustra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From TS point of view it looks good, however I see a lot of changes in the patches so I'm going to summon a nav expert @WojtekBoman
patches/react-navigation/@react-navigation+stack+7.6.11+001+edge-drag-gesture.patch
Show resolved
Hide resolved
No access. I will be creating PR for oldDot
'No QA' makes sense for QA team as this is full regression test but @TaduJR and I will do thorough testing. |
I think it should be created alongside this one, so that both are merged at the same time @aimane-chnaif |
|
I see, the patch |
|
Thanks for the feedback. Agree to add test case (#65709) for missing patch. (though I see that it's fixed in upstream react-navigation/react-navigation#12679) @TaduJR let's address all above comments. |
I see that in many places the patches look the same as before. As Błażej noticed, comments were removed from the patch I would suggest to test flows fixed by the patches. They can be found in If any changes were made in the patches, I would suggest including this in the PR description, for example you can describe why the patch |
|
Added Demos. |
|
@TaduJR |
|
ok, can you add notes for context or logic changes? I got confused because you didn't add any notes. |
|
Updated the PR Description @aimane-chnaif |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb Chromemchrome.moviOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #75700 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@chiragsalian can you please generate adhoc builds before merge? |
|
🚧 @chiragsalian has triggered a test Expensify/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! 🧪🧪
|
|
Weird bug in tab bar while swiping: swipe.mp4(We're disabling tab swipe behavior here so might not be a blocker) |
chiragsalian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks fine to me. Anyone else wants to final review - @WojtekBoman @blazejkustra @aimane-chnaif?
Or shall we go ahead and merge this. Thoughts?
|
Let's hold a bit for #76343 which will likely to be merged soon. |
|
It's merged. We can merge |
|
@TaduJR please merge main as branch is too behind |
|
✋ 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/chiragsalian in version: 9.2.87-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.87-7 🚀
|
Explanation of Change
This PR upgrades react-navigation packages to their latest versions for React Native 0.82 compatibility:
@react-navigation/native: 7.1.10 → 7.1.24@react-navigation/stack: 7.3.3 → 7.6.11react-native-safe-area-context: 5.4.0 → 5.6.2Patch changes:
core+platform-navigation-stack-typesScreenOptionsOrCallbackandConvertCustomScreenOptionstypes to TypeScript declarations;useNavigationBuildernow has 6th type parameter and optional 3rd parametercore+fix-clearing-preloaded-routes-after-logoutstack+edge-drag-gesturestack+dontDetachScreencore+fix-crash-when-parsing-emojicore+fix-failing-jest-by-disabling-esmodulenative+initialnative+fix-failing-jest-by-disabling-esmodulerouters+fix-failing-jest-by-disabling-esmodulestack+fix-failing-jest-by-disabling-esmodulenative-stack+added-interaction-manager-integrationnative-stack+fix-failing-jest-by-disabling-esmoduleelements+fix-failing-jest-by-disabling-esmodulematerial-top-tabs+fix-failing-jest-by-disabling-esmoduleType fixes:
ScreenOptionsOrCallbackis no longer exported from@react-navigation/nativein v7.13.5+, so we define it locally inPlatformStackNavigation/types/index.tsDefaultNavigatorOptionsusage from 7 to 6 type arguments (breaking change in react-navigation)Test fix:
useNavigationStateinBottomTabBarTest.tsxto handle behavior changes in the new versionFixed Issues
$ #75700
PROPOSAL: #75700 (comment)
Tests
Offline tests
Same as tests
QA Steps
Same as tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
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-Native.mov
Android: mWeb Chrome
Android-mWeb.mov
iOS: Native
iOS-Native.mov
iOS: mWeb Safari
iOS-Safari.mov
MacOS: Chrome / Safari
Mac-Chrome.mov