-
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
[HybridApp] Fix NewDot internal links in HybridApp #36280
Conversation
@parasharrajat 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] |
Wooot! Rajat to review! |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
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.
The change itself looks good.
I tested with the mobile AdHoc build and this the staging deep-link is working well 👍
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-02-14.at.12.19.40.AM.movAndroid: mWeb ChromeScreen.Recording.2024-02-13.at.11.57.29.PM.moviOS: NativeScreen.Recording.2024-02-14.at.12.27.36.AM.moviOS: mWeb SafariScreen.Recording.2024-02-13.at.11.56.16.PM.movMacOS: Chrome / SafariScreen.Recording.2024-02-13.at.11.53.29.PM.movMacOS: DesktopScreen.Recording.2024-02-13.at.11.55.09.PM.mov |
@allroundexperts can you prioritize this one when you have a moment? Thank youuu! ❤️ |
@dylanexpensify Just 10 more mins! |
Okay. Update: Still trying to build the apps locally. |
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.
Tests well!
✋ 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/AndrewGable in version: 1.4.42-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.4.42-5 🚀
|
cc: @AndrewGable
Details
The changes required to fix wrong behaviour of NewDot in the HybridApp. The fixed issue was a side effect of a bigger problem caused by wrong integration of react-native-config package. The package was not able to find .env files, and as a result the app behaviour in some cases could be incorrect.
In the case of fixed issue associated domains didn't match, and tapping on links (in the issue's case a link to a room) caused opening web browser, instead of navigating within the app. In fact all "internal" links didn't work as expected.
The purpose of the patch is to allow specifying path to a react native project in a custom folder structure. The variables which are used are specific to our project. However, in the future I plan to introduce more universal solution, which would allow us to remove it.
The PR introduces just a part of required changes. The PR with changes to OldDot is available here (only for Expensify OldDot contributors).
Fixed Issues
$ #34378
PROPOSAL: #34378
Tests
WARNING: This PR is HybridApp-specific, and should not introduce any changes to NewDot. The bug exists only on HybridApp, therefore the following steps are made to make sure, everything works fine.
IMPORTANT: Use correct link for correct environment:
dev: https://dev.new.expensify.com:8082/concierge
staging: https://staging.new.expensify.com/concierge
production: https://new.expensify.com/concierge
See if concierge chat was opened
Offline tests
QA Steps
WARNING: This PR is HybridApp-specific, and should not introduce any changes to NewDot. The bug exists only on HybridApp, therefore the following steps are made to make sure, everything works fine.
IMPORTANT: Use correct link for correct environment:
dev: https://dev.new.expensify.com:8082/concierge
staging: https://staging.new.expensify.com/concierge
production: https://new.expensify.com/concierge
See if concierge chat was opened
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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label 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
rn_image_picker_lib_temp_45427a4e-ee18-45de-872f-29a3fb93262e-2024-02-09.18_20_34.550.mp4
Android: mWeb Chrome
Screen.Recording.2024-02-12.at.07.37.08.mov
iOS: Native
RPReplay_Final1707491435-2024-02-09.18_20_24.839.mp4
iOS: mWeb Safari
rn_image_picker_lib_temp_45427a4e-ee18-45de-872f-29a3fb93262e-2024-02-09.18_20_34.550.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-02-12.at.07.30.04.mov
MacOS: Desktop
Screen.Recording.2024-02-12.at.07.34.17.mov