-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Migrate E/App to use PlatformStackNavigation
#49937
base: main
Are you sure you want to change the base?
Migrate E/App to use PlatformStackNavigation
#49937
Conversation
@srikarparsi 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] |
PlatformStackNavigation
in E/App (introduce native-stack
navigation on native)PlatformStackNavigation
in E/App (introduce native-stack
navigation on mobile)
PlatformStackNavigation
in E/App (introduce native-stack
navigation on mobile)PlatformStackNavigation
PlatformStackNavigation
PlatformStackNavigation
3b921d3
to
5701c46
Compare
Re-assigning to @chiragsalian and @mountiny for review since it looks like they have more context. |
@mountiny or @chiragsalian please from now on create ad-hoc builds from here. I fixed all remaining reported errors yesterday, so we can probably trigger a new build already. |
This comment has been minimized.
This comment has been minimized.
The ad-hoc iOS build fail is also going to be related to the patch from #49936 |
@mountiny Please run adhoc builds here when you have a moment, i plan to work on testing this fully today |
Running |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
The PR didn't have the changes to the |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@chrispader Looking great! I think there are two things I have noticed:
|
package-lock.json
Outdated
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.
commenting so this shows on my k2 for easy access
@mountiny Both bugs are fixed! I couldn't really see a difference between Stack navigator and Native-Stack while testing in this PR: (you meant when clicking on an item in the Sidebar right?) StackSimulator.Screen.Recording.-.iPhone.15.Pro.-.2024-10-06.at.17.38.30.mp4Native StackSimulator.Screen.Recording.-.iPhone.15.Pro.-.2024-10-06.at.20.09.51.mp4 |
I will be OOO the next week, but i will hand this over to one of our other Margelo engineers. If there are any urgent issues i can also help with that, but i'm not gonna be on my Macbook unfortunately |
I am now available for around ~2 hours i'll pull and start testing this locally |
🚧 @mountiny 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! 🧪🧪 |
Cannot reproduce this. On iOS mWeb and macOS web the last ad-hoc builds load just fine for me (maybe this was caused by the staging server being down recently?) |
Checked with latest build, it works fine now 👍 |
Opening threads from specific room chat( https://49937.pr-testing.expensify.com/r/6776919265133947) seems to be crashing the app Screen.Recording.2024-11-21.at.10.53.15.PM.mov |
Can you merge main for a new build? might be coming from that. also can you start checking the Prettier and ESLint checks (unless its the useOnyx migration) so we can have this ready to merge |
@mountiny can you trigger build again please |
with this build #49937 (comment), Ios App is also crashing randomly, No specific steps just happened 2 times while using the app after clear cache and restart. Edit: i got this crash on Staging App also, so not related to this PR |
Merged main! We still rely on the changes from #52392 to fix the "Android screen cut off" issue, so i guess we should better hold for that PR? ESLint and Prettier workflows look good! |
🚧 @mountiny 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! 🧪🧪 |
Sadly this is producable for me again #49937 (comment)
Also similar issue was producable once just on a different screen (setting > Wallet > Enable Wallet) but i could not record it happening Screen.Recording.2024-11-22.at.8.04.56.PM.movOther than this i played around the app everything looks and feels good to me 👌 |
I reverted the merge of the Android edge-to-edge PR, so this is expected atm. I'm currently investigating whether we just need a part of the changes from the PR or if we should hold on the PR to get merged first. We definitely don't want all the changes from the other PR being added here. @ishpaul777 if you didn't find any other bugs, we might have fixed all current problems for now. I will try to get the "Android issue screen cut-off issue" sorted out in the next hour or so |
Also please check if you can reproduce #49937 (comment), because this is also consistently reprocable for me. |
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
Just merged the changes from The last issue (that i know of) that i'm currently working on is this: #49937 (comment) |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@ishpaul777 can you please retest the keyboard issue on android? |
On it 👀 |
@mountiny @chrispader Issue is fixed 🎉 |
yay |
@mountiny @ishpaul777 @chiragsalian
Details
This PR migrates the App to use
PlatformStackNavigation
types and navigator components implemented in #37891Fixed Issues
$ #37891
PROPOSAL: #37891 (comment)
Tests
Open app
press on search filed
be sure that:
close screen
open any chat
tap on composer
go to previous screen
be sure there is no keyboard jumps on previous screen -> [HOLD for payment 2024-10-17] [HOLD for payment 2024-06-06] [HOLD #37891] Keyboard jumps around when swiping back from chat to LHN with gesture #37923
open chat again
tap on task
tap on Assignee
be sure there is no infinite loading for the screen -> [HOLD for payment 2024-03-07] Android - Task-Tapping assignee shows infinite loading page #37252
go back to task
tap on Title
be sure keyboard is opened automatically -> [HOLD for payment 2024-03-07] [$500] Android - Task - Keyboard does not open after selecting task title #37273
go back to main page
tap on user avatar (to open Account settings)
tap on wallet, tap on "Add bank account"
tap on "Debit card"
be sure keyboard gets opened automatically -> [HOLD for payment 2024-03-07] [$500] Android - Wallet - The keyboard does not open automatically when the debit card is opened #37325
go back to main page
press "+" in TabBar
tap Request Money
enter any amount
press "Next"
be sure that on participant screen keyboard appears from the bottom -> [HOLD for payment 2024-03-07] [$500] IOU - Keyboard comes out from right to left on participant screen #37257
Verify that no errors appear in the JS console
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop