-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Perf/native stack navigation #29991
Perf/native stack navigation #29991
Conversation
@abdulrahuman5196 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] |
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.
cc @adamgrzybowski @WoLewicki could you please review too, look good to me but I think we need to have a discussion about the animations
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
@mountiny has noted two issues on iOS:
Flickering issue There was also this issue on the react-native repo, but it was closed, as its a native iOS issue: facebook/react-native#39411 Native iOS flickering issue (no workaround available): native-ios-flickering.mp4Black keyboard Native iOS keyboard issue in dark mode (checking if a workaround is possible): native-ios-black-keybioard.mp4 |
@hannojg Just gentle check on the status of this issue. |
Hey, no update so far, we still have to see if we find a mitigation for the native iOS bugs with keyboard. Unknown timeline for that sorry 🥶 |
@hannojg Any updates on this one? Could we sync up with main and see how this looks like now? |
Kiryl will take this one over next week!! |
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-02-21.at.11.40.39.PM.mp4Android: mWeb ChromeScreen.Recording.2024-02-21.at.11.38.25.PM.mp4iOS: NativeScreen.Recording.2024-02-21.at.11.29.48.PM.mp4iOS: mWeb SafariScreen.Recording.2024-02-21.at.11.21.49.PM.mp4MacOS: Chrome / SafariScreen.Recording.2024-02-21.at.11.03.01.PM.mp4MacOS: DesktopScreen.Recording.2024-02-21.at.11.07.18.PM.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.
Changes looks good and works well. Reviewers checklist is also complete.
All yours. @mountiny / @Julesssss
🎀 👀 🎀
C+ Reviewed
✋ 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/Julesssss in version: 1.4.44-0 🚀
|
I suggest to revert this PR until |
@situchan I agree with you - I'll create revert PR soon 👀 |
@mountiny may I kindly ask you to revert this PR? I think you should have a button "revert" at the bottom of the page? I'll re-open a PR with all fixes later 👀 |
Straight revert didn't work as there's conflict. Should be reverted manually |
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.44-13 🚀
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.4.45-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.45-6 🚀
|
Details
Change for the mobile platforms to use the native-stack navigator:
With this we will get native screen transitions, which feel better and don't get blocked if the JS thread is blocked.
This was discussed here: #vip-newdot-performance
Changes:
@react-navigation/native-stack
src/libs/Navigation/PlatformStackNavigation
created files for mobile / other platforms that expose the correct navigation packageNote: In some places I didn't explicitly split the options for
stack
andnative-stack
usage. Simply because on thenative-stack
we often times don't need the options that get set, because we rely on the platform default configuration. In addition the names for the options fromstack
don't exist innative-stack
, so they will never really get applied innative-stack
.However, this is more of a design decision. I felt that it will keep the code cleaner, than to separate the options for everything (which means creating two files, where for native it's just an empty object).
Examples of such places:
https://github.com/margelo/expensify-app-fork/blob/27b2a131ccc8d7a9652adc127f67752f6911b447/src/libs/Navigation/AppNavigator/getRootNavigatorScreenOptions.js#L25-L27
Fixed Issues
$ #29948
PROPOSAL: #29948
Tests
Offline tests
n/a
QA Steps
PR 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)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)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
trim.33285B99-69AB-4C2A-A141-B619A4D830C4.MOV
Android: mWeb Chrome
mweb-android.mov
iOS: Native
Simulator_Screen_Recording_-_iPhone_15_Pro_-_2024-02-16_at_17.24.23.mp4
iOS: mWeb Safari
mweb-ios.mp4
MacOS: Chrome / Safari
chrome.mov
MacOS: Desktop
desktop.mov