-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[Travel] Bring back refactored AddressPage
#43348
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
[Travel] Bring back refactored AddressPage
#43348
Conversation
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-06-27.at.5.16.02.in.the.afternoon.movAndroid: mWeb ChromeScreen.Recording.2024-06-17.at.2.14.53.in.the.afternoon.moviOS: NativeScreen.Recording.2024-06-27.at.5.13.45.in.the.afternoon.moviOS: mWeb SafariScreen.Recording.2024-06-18.at.12.25.13.in.the.afternoon.movMacOS: Chrome / SafariScreen.Recording.2024-06-17.at.2.08.07.in.the.afternoon.movMacOS: DesktopScreen.Recording.2024-06-18.at.12.33.13.in.the.afternoon.mov |
|
@mateuuszzzzz is there any fix we applied to fix #42638 other than bringing back #41970? i can't seem to find it |
|
@getusha It's the same code, context in the PR's description: |
|
Thanks @blazejkustra! i missed that 😄 |
|
Screen.Recording.2024-06-12.at.2.26.15.in.the.afternoon.mov |
Is this reproducible on main? |
|
@blazejkustra not reproducible on Screen.Recording.2024-06-13.at.10.57.22.in.the.morning.mov |
@getusha I'll take a look at this bug this evening 👌 |
|
@getusha it should work now demo.mov |
|
I noticed #42638 is still kind of reproducible, i am not able to repro it consistently Screen.Recording.2024-06-14.at.6.59.27.in.the.morning.mov |
It's very specific bug. I was trying to reproduce the steps that you showed on the demo but I am not able to do it. I will introduce previous fix and we can check once again if problem disappeared. |
|
@getusha ready |
|
Screen.Recording.2024-06-18.at.12.47.02.in.the.afternoon.movNot sure, if this is reproducible on main/staging will try it & only able to reproduce it on |
|
hello 😊 Recently Mateusz has had time-consuming tasks connected to hybrid app, so I will take over this PR. |
I've managed to reproduce this issue and I'll be looking for a solution. |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
hi @getusha 😊
The bug was appearing every time when country was changed. I think now it's solved, can you check out if it's working for you? |
src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx
Outdated
Show resolved
Hide resolved
|
@zfurtak friendly bump |
mountiny
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.
Thank you for getting this done
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.5-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.5-2 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.5-13 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀
|

Details
PR brings back refactored AddressPage which had a bug with
CountrySelectorbefore.Initially fix supposed to changeuseMemodependencies inAddressPage. Nevertheless, on the latest mainAddressPageworks again without those changes so I'm leaving it as it was implemented initially.Bug is still reproducible but it's harder to reproduce it. So the fix was reintroduced.
NOTE: We have another bug on main, related to
AddressPagethat is not addressed in this PR.Fixed Issues
$ #42638
$ #41965
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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.mWeb.mov
MacOS: Chrome / Safari
Browser.mov
MacOS: Desktop
Deskop.mov