-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Update @react-navigation/native version #32087
Conversation
@@ -83,7 +83,7 @@ | |||
"@react-native-google-signin/google-signin": "^10.0.1", | |||
"@react-native-picker/picker": "^2.4.3", | |||
"@react-navigation/material-top-tabs": "^6.6.3", | |||
"@react-navigation/native": "6.1.6", | |||
"@react-navigation/native": "6.1.8", |
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.
@dukenv0307, have you encountered this prompt?
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.
Do I need to create a new patch?
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.
Ah, I think so, and we should avoid introducing regressions. :)
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.
@ntdiary Updated with new patch.
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.
Comparing v6.1.6 and v6.1.8, the file createMemoryHistory.js
is same, and the differences in useLinking.js
are only the series
function and one console.warn
statement. 🤔
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.
@dukenv0307, curious how you recreated this patch. I applied the two patches separately, and it seems they have some significant differences compared to the previous ones. 😂
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.
@ntdiary I will go to the node_module file of the lib and compare it with the old patch. Some code in useLinking.js
is added in the new version of the lib which is the same as old patch so we don't need it in the new patch file.
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.
@dukenv0307, I thought you would check the patch again to make sure it is correct. Based on the two images I provided above, it is clear that the changes were not correctly synchronized to the new version. Can you please check that again? 😅
For example, in the old patch, the line index = this.index;
was placed inside the onPopState()
function, but in the new patch, it was placed above the onPopState()
function.
In the old patch, the comment // and remove any entries
was added after the comment // If history length
, but in the new patch, it was placed after the comment // Store the updated
. These differences will cause the function logic to change.
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.
@ntdiary Thanks for your review, updated.
@ntdiary Any update here. |
74a24bc
to
4b24778
Compare
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-native.mp4Android: mWeb Chromeandroid-chrome.mp4iOS: Nativeios-app.mp4iOS: mWeb Safariios-safari.mp4MacOS: Chrome / Safariweb.mp4MacOS: Desktopdesktop.mp4 |
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.
LGTM. :)
Snyk's failed because of a broken package-lock check so I skipped it |
✋ 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/arosiclair in version: 1.4.13-0 🚀
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 1.4.13-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.13-8 🚀
|
Details
Upgrade
@react-navigation/native
to version6.1.8
to preventBackHandler
errorFixed Issues
$ #31487
PROPOSAL: #31487 (comment)
Tests
Setting
pageBackHandler
console error doesn't appearOffline tests
Same as above
QA Steps
Setting
pageBackHandler
console error doesn't appearPR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.- [x] I linked the correct issue in the### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2023-11-28.at.12.03.12.mov
Android: mWeb Chrome
Screen.Recording.2023-11-28.at.11.57.33.mov
iOS: Native
Screen.Recording.2023-11-28.at.12.04.43.mov
iOS: mWeb Safari
Screen.Recording.2023-11-28.at.12.00.42.mov
MacOS: Chrome / Safari
Screen.Recording.2023-11-28.at.11.52.57.mov
MacOS: Desktop
Screen.Recording.2023-11-28.at.12.10.02.mov