[TS migration] Migrate 'IOUWaypoint' page to TypeScript #33244
Conversation
…s-migration/IOUWaypoint/page
…s-migration/IOUWaypoint/page
…s-migration/IOUWaypoint/page
|
@blazejkustra kindly bump 😄 |
…s-migration/IOUWaypoint/page
|
@blazejkustra ready to recheck 😄 |
…s-migration/IOUWaypoint/page
|
@blazejkustra kindly bump 😄 |
| return ( | ||
| // @ts-expect-error TODO: Remove this once withFullTransactionOrNotFound is migrated to TypeScript. |
There was a problem hiding this comment.
Link the issue if it exists (same for other comments)
| // @ts-expect-error TODO: Remove this once withFullTransactionOrNotFound is migrated to TypeScript. | |
| // @ts-expect-error TODO: Remove this once withFullTransactionOrNotFound is migrated to TypeScript. |
There was a problem hiding this comment.
there are no issues, at least in spreadsheet
There was a problem hiding this comment.
+1 Let's remove todo and link issue. Here is the issue #36123
Also can you please comment there so whoever picks up can clean this up.
| type IOURequestStepWaypointProps = { | ||
| route: { | ||
| params: { | ||
| iouType: ValueOf<typeof CONST.IOU.TYPE>; | ||
| transactionID: string; | ||
| reportID: string; | ||
| backTo: Routes | undefined; | ||
| action: ValueOf<typeof CONST.IOU.ACTION>; | ||
| pageIndex: string; | ||
| }; | ||
| }; | ||
| transaction: OnyxEntry<OnyxTypes.Transaction>; | ||
| } & IOURequestStepWaypointOnyxProps; |
There was a problem hiding this comment.
Should we use StackScreenProps here?
| type IOURequestStepWaypointProps = { | |
| route: { | |
| params: { | |
| iouType: ValueOf<typeof CONST.IOU.TYPE>; | |
| transactionID: string; | |
| reportID: string; | |
| backTo: Routes | undefined; | |
| action: ValueOf<typeof CONST.IOU.ACTION>; | |
| pageIndex: string; | |
| }; | |
| }; | |
| transaction: OnyxEntry<OnyxTypes.Transaction>; | |
| } & IOURequestStepWaypointOnyxProps; | |
| type IOURequestStepWaypointProps = IOURequestStepWaypointOnyxProps & StackScreenProps<...> |
There was a problem hiding this comment.
Actually no, because StackSceenProps is providing more properties which are passed to this component
<IOURequestStepWaypoint
// Put the transactionID into the route params so that WaypointEdit behaves the same when creating a new waypoint
// or editing an existing waypoint.
route={{
params: {
...route.params,
transactionID,
},
}}
/>
…s-migration/IOUWaypoint/page
…s-migration/IOUWaypoint/page
|
@abdulrahuman5196 yeah you are right its not needed , its fixed now so you can recheck |
|
@kubabutkiewicz Still I am seeing changes in Gemfile on this PR. Could you kindly remove the changes? |
|
@abdulrahuman5196 done |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-02-20.at.10.39.08.PM.mp4Android: mWeb ChromeScreen.Recording.2024-02-20.at.10.41.17.PM.mp4iOS: NativeScreen.Recording.2024-02-20.at.10.52.17.PM.mp4iOS: mWeb SafariScreen.Recording.2024-02-20.at.10.44.56.PM.mp4MacOS: Chrome / SafariScreen.Recording.2024-02-20.at.10.57.31.PM.mp4MacOS: DesktopScreen.Recording.2024-02-20.at.10.55.34.PM.mp4 |
There was a problem hiding this comment.
Changes looks good and works well. Reviewers checklist is also complete.
All yours. @MonilBhavsar
🎀 👀 🎀
C+ Reviewed
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #31996 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
FYI @MonilBhavsar is assigned to this PR for internal review. |
|
Gentle ping on this @MonilBhavsar |
|
On my radar. I'll get to this 😄 |
Thank you. Just wanted to avoid merge conflicts. |
| return ( | ||
| // @ts-expect-error TODO: Remove this once withFullTransactionOrNotFound is migrated to TypeScript. |
There was a problem hiding this comment.
+1 Let's remove todo and link issue. Here is the issue #36123
Also can you please comment there so whoever picks up can clean this up.
| }, | ||
| })), | ||
| }, | ||
| // @ts-expect-error TODO: Remove this once SettlementButton (https://github.com/Expensify/App/issues/25100) is migrated to TypeScript. |
There was a problem hiding this comment.
#25100 is closed. Can we remove this now?
There was a problem hiding this comment.
That was a mistake its waiting for #36123 , adjusted a comment, I think there is no need to put comment on the issue cause when withFullTransactionOrNotFound will be migrated this line will produce TS error that this comment is not needed anymore.
…s-migration/IOUWaypoint/page
|
@MonilBhavsar I answered/resolved comments 😄 |
|
Thanks! I can approve but there's unstoppable - conflict |
…s-migration/IOUWaypoint/page
|
@MonilBhavsar fixed! |
|
✋ 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/MonilBhavsar in version: 1.4.44-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.44-13 🚀
|
| lat: null, | ||
| lng: null, |
There was a problem hiding this comment.
Hey there,
I think this change caused a regression where offline addresses are not saved, lat and lng are required props that are used to decide whether to save a waypoint or not, specifically this block:
App/src/libs/actions/Transaction.ts
Lines 90 to 95 in 6065c4e
There was a problem hiding this comment.
will take a look @fedirjh
There was a problem hiding this comment.
@fedirjh do you know if there is issue for this?
I have a code ready to create a PR
There was a problem hiding this comment.
do you know if there is issue for this?
I don't think we have any ticket to track this.
cc @puneetlath Can you please create an issue for this one? or should I report it to the bugs channel?
There was a problem hiding this comment.
@fedirjh if you could report it that'd be great. Thanks!
| name: values.name || null, | ||
| name: values.name, |
There was a problem hiding this comment.
This change caused a regression #37640
Details
Fixed Issues
$ #31996
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.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 so 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
waypoint-android.mp4
Android: mWeb Chrome
iOS: Native
waypoint-ios.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
waypoint-web.mp4
MacOS: Desktop
waypoint-desktop.mp4