Chore: Remove getStateFromPath patch#65072
Conversation
|
|
Not really, in the patch there were only JS changes |
|
@allgandalf 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] |
|
Important: While testing you may notice that selecting a location doesn't work perfectly - this will be fixed by this PR by @adamgrzybowski: #65077, so it may be ignored for the testing here until it lands on |
|
After merging the fix this PR is ready for review! |
|
@allgandalf can you please test? |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
WojtekBoman
force-pushed
the
chore/remove-getstatefrompath-patch
branch
from
12fec35 to
53600d2
Compare
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-07-03.at.10.16.03.PM.movAndroid: mWeb ChromeScreen.Recording.2025-07-03.at.10.11.28.PM.moviOS: HybridAppScreen.Recording.2025-07-03.at.10.16.36.PM.moviOS: mWeb SafariScreen.Recording.2025-07-03.at.10.17.19.PM.movMacOS: Chrome / SafariScreen.Recording.2025-07-03.at.9.49.35.PM.movMacOS: DesktopScreen.Recording.2025-07-03.at.10.12.39.PM.mov |
|
@allgandalf let me know once done! thanks |
|
started testing now, ~1 hour from now |
| MONEY_REQUEST_CREATE_TAB_DISTANCE: { | ||
| route: ':action/:iouType/start/:transactionID/:reportID/distance/:backToReport?', | ||
| route: 'distance/:backToReport?', | ||
| getRoute: (action: IOUAction, iouType: IOUType, transactionID: string, reportID: string, backToReport?: string) => | ||
| `create/${iouType as string}/start/${transactionID}/${reportID}/distance/${backToReport ?? ''}` as const, | ||
| }, | ||
| MONEY_REQUEST_CREATE_TAB_MANUAL: { | ||
| route: ':action/:iouType/start/:transactionID/:reportID/manual/:backToReport?', | ||
| route: 'manual/:backToReport?', | ||
| getRoute: (action: IOUAction, iouType: IOUType, transactionID: string, reportID: string, backToReport?: string) => | ||
| `${action as string}/${iouType as string}/start/${transactionID}/${reportID}/manual/${backToReport ?? ''}` as const, | ||
| }, | ||
| MONEY_REQUEST_CREATE_TAB_SCAN: { | ||
| route: ':action/:iouType/start/:transactionID/:reportID/scan/:backToReport?', | ||
| route: 'scan/:backToReport?', |
There was a problem hiding this comment.
@staszekscp I don't understand this change, This is a major route change imo, did we get this approved? so from now for manual request instead of the route : create/create/start/1/3269280911335422/manual, we will have a simple manual route in the URL? , is this assumption correct?, also is this the same for quick actions, what are the steps exactly to test this url change?
There was a problem hiding this comment.
The route field is used in the linking config. The difference now is that we don't use path: exact. We need to do this because some parameters need to be passed to the screen and some to its parent navigator.
You can see that this part :action/:iouType/start/:transactionID/:reportID/distance is common for all screens. We used this path for the parent navigator as path exact and added the rest in the config for its screens, like distance/:backToReport?
You can see that getRoute still has the whole create/${iouType as string}/start/${transactionID}/${reportID}/distance/${backToReport ?? ''}
TLDR: The visible URL in the browser will stay the same. We only changed where the params will be available from useRoute()
There was a problem hiding this comment.
@adamgrzybowski So to confirm, I believe we user the full create/${iouType as string}/start/${transactionID}/${reportID}/distance/${backToReport ?? ''} in some messaging to deeplink to the flow. That will still work?
There was a problem hiding this comment.
Checked and its using /start/submit/distance so should be ok
| MONEY_REQUEST_CREATE_TAB_DISTANCE: { | ||
| route: ':action/:iouType/start/:transactionID/:reportID/distance/:backToReport?', | ||
| route: 'distance/:backToReport?', | ||
| getRoute: (action: IOUAction, iouType: IOUType, transactionID: string, reportID: string, backToReport?: string) => | ||
| `create/${iouType as string}/start/${transactionID}/${reportID}/distance/${backToReport ?? ''}` as const, | ||
| }, | ||
| MONEY_REQUEST_CREATE_TAB_MANUAL: { | ||
| route: ':action/:iouType/start/:transactionID/:reportID/manual/:backToReport?', | ||
| route: 'manual/:backToReport?', | ||
| getRoute: (action: IOUAction, iouType: IOUType, transactionID: string, reportID: string, backToReport?: string) => | ||
| `${action as string}/${iouType as string}/start/${transactionID}/${reportID}/manual/${backToReport ?? ''}` as const, | ||
| }, | ||
| MONEY_REQUEST_CREATE_TAB_SCAN: { | ||
| route: ':action/:iouType/start/:transactionID/:reportID/scan/:backToReport?', | ||
| route: 'scan/:backToReport?', |
There was a problem hiding this comment.
Checked and its using /start/submit/distance so should be ok
|
🚧 @mountiny has triggered a test Expensify/App 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! 🧪🧪
|
|
I did try to use https://65072.pr-testing.expensify.com/start/submit/distance route and it did not work/ did not show me the start of the distance flow but same on staging |
|
✋ 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/mountiny in version: 9.1.77-1 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.1.77-2 🚀
|
5 participants
cc: @mountiny
Explanation of Change
Follow up to
@react-navigationbump that removes one more patch. I had to modify the linking config, so the path is notexact, but is a result of path concatenation in the react navigation internals. This way we get a correct state from thegetStateFromPathfunction.Fixed Issues
$ #62850
PROPOSAL: N/A
Tests
Create Expenseand verify if the user can navigate correctly back and forthOffline tests
QA Steps
Create Expenseand verify if the user can navigate correctly back and forthPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)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
Screen.Recording.2025-06-27.at.10.02.51.mov
Android: mWeb Chrome
Screen.Recording.2025-06-27.at.11.08.05.mov
iOS: Native
Screen.Recording.2025-06-27.at.10.55.56.mov
iOS: mWeb Safari
Screen.Recording.2025-06-27.at.11.02.37.mov
MacOS:
Screen.Recording.2025-06-27.at.11.07.07.mov
hrome / Safari
MacOS: Desktop
Screen.Recording.2025-06-27.at.11.12.39.mov