[TS migration] Migrate withFullTransactionOrNotFound HOC #36855
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fabioh8010
reviewed
Feb 20, 2024
|
|
||
| type WithFullTransactionOrNotFoundOnyxProps = { | ||
| /** Indicated whether the report data is loading */ | ||
| transaction: OnyxCollection<Transaction>; |
There was a problem hiding this comment.
Suggested change
| transaction: OnyxCollection<Transaction>; | |
| transaction: OnyxEntry<Transaction>; |
| export default function <TProps extends WithFullTransactionOrNotFoundProps, TRef>(WrappedComponent: ComponentType<TProps & RefAttributes<TRef>>) { | ||
| // eslint-disable-next-line rulesdir/no-negated-variables | ||
| function WithFullTransactionOrNotFound(props: TProps, ref: ForwardedRef<TRef>) { | ||
| const transactionID = props.route.params.transactionID; |
There was a problem hiding this comment.
Suggested change
| const transactionID = props.route.params.transactionID; | |
| const transactionID = props.transaction?.transactionID; |
Let's be cautious with these changes, before the file was getting from props 👀
| key: ({route}) => { | ||
| const transactionID = route.params.transactionID ?? 0; | ||
| const userAction = route.params.action ?? CONST.IOU.ACTION.CREATE; | ||
| return `${userAction === CONST.IOU.ACTION.CREATE ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`; |
There was a problem hiding this comment.
Suggested change
| return `${userAction === CONST.IOU.ACTION.CREATE ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`; | |
| if (userAction === CONST.IOU.ACTION.CREATE) { | |
| return `${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}` as `${typeof ONYXKEYS.COLLECTION.TRANSACTION}${string}`; | |
| } | |
| return `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`; |
Isn't pretty solution but I'm afraid we don't have other options
fabioh8010
reviewed
Feb 20, 2024
| }, | ||
| }, | ||
| })(forwardRef(WithFullTransactionOrNotFound)); | ||
| } |
There was a problem hiding this comment.
Suggested change
| } | |
| } | |
| export type {WithFullTransactionOrNotFoundProps}; |
Also, let's export the type (it will be used in the future)
fabioh8010
approved these changes
Feb 20, 2024
blazejkustra
approved these changes
Feb 27, 2024
Reviewer Checklist
Screenshots/VideosAndroid: NativeCleanShot.2024-02-28.at.23.41.25.mp4Android: mWeb ChromeCleanShot.2024-02-28.at.23.44.10.mp4iOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-28.at.23.54.33.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-28.at.23.56.10.mp4MacOS: Chrome / SafariCleanShot.2024-02-28.at.23.20.18.mp4MacOS: DesktopCleanShot.2024-02-28.at.23.52.27.mp4 |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #36123 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
fedirjh
approved these changes
Feb 28, 2024
tylerkaraszewski
approved these changes
Feb 29, 2024
36 tasks
|
🚀 Deployed to staging by https://github.com/tylerkaraszewski in version: 1.4.46-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.4.46-2 🚀
|
|
Hey @ruben-rebelo This PR was reverted, are you working on this, right? |
|
@fabioh8010 Yes, that is correct! |
|
Any updates @ruben-rebelo ? |
|
Just created an draft pr getting this back with the issue fixes, here is the PR: 38990 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
[TS migration] Migrate *withFullTransactionOrNotFound HOC to TypeScript
Fixed Issues
$ #36123
PROPOSAL: N/A
Tests
Verify that no errors appear in the JS console
Open the app and log in
Navigate to any chat
Press plus and request money
Following the request money process should work as before
Offline tests
N/A
QA Steps
Same as in the Tests section.
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
8mb.video-2W7-OcZXZfPw.mp4
Android: mWeb Chrome
android-web.mov
iOS: Native
ios-native.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
macos-web.mov
MacOS: Desktop
macos-native.mov