[TS migration] Migrate 'MoneyRequestConfirmationList.js' component to TypeScript#34168
Conversation
…s-migration/MoneyRequestConfirmationList/component
…s-migration/MoneyRequestConfirmationList/component
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| onPress={(_event, value) => confirm(value)} |
There was a problem hiding this comment.
| // eslint-disable-next-line @typescript-eslint/naming-convention | |
| onPress={(_event, value) => confirm(value)} | |
| onPress={(event, value) => confirm(value)} |
There was a problem hiding this comment.
I think this underscore was added for this marking this param as not used so not sure if this will change anything since I think we will still need disable eslint for that line
…s-migration/MoneyRequestConfirmationList/component
| type ConfirmedRouteProps = ConfirmedRoutePropsOnyxProps & { | ||
| /** Transaction that stores the distance request data */ | ||
| transaction: Transaction; | ||
| transaction: Transaction | undefined; |
There was a problem hiding this comment.
| transaction: Transaction | undefined; | |
| transaction?: Transaction; |
…s-migration/MoneyRequestConfirmationList/component
| bankAccountRoute = '', | ||
| policyID = '', | ||
| reportID = '', | ||
| receiptPath = '', | ||
| receiptFilename = '', | ||
| transactionID = '', |
There was a problem hiding this comment.
If possible let's remove empty strings as default value
…s-migration/MoneyRequestConfirmationList/component
melvin-bot
Bot
requested review from
hoangzinh
and removed request for
a team
|
@hoangzinh 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] |
…s-migration/MoneyRequestConfirmationList/component
| ? translate('common.tbd') | ||
| : CurrencyUtils.convertToDisplayString( | ||
| shouldCalculateDistanceAmount | ||
| ? DistanceRequestUtils.getDistanceRequestAmount(distance, mileageRate?.unit ?? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, mileageRate?.rate ?? 0) |
There was a problem hiding this comment.
Could you explain why don't we set default value for mileageRate like previous
Then we can just use mileageRate.unit here
There was a problem hiding this comment.
Right, but when we default value in props for some reason typescript is not detecting that and for example type for mileageRate?.unit is still Unit | undefined
| return; | ||
| } | ||
|
|
||
| const amount = DistanceRequestUtils.getDistanceRequestAmount(distance, mileageRate?.unit ?? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, mileageRate?.rate ?? 0); |
There was a problem hiding this comment.
| style={[styles.moneyRequestMenuItem, styles.mt2]} | ||
| titleStyle={styles.moneyRequestConfirmationAmount} | ||
| disabled={didConfirm} | ||
| brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isAmountMissing(transaction ?? null) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined} |
There was a problem hiding this comment.
The previous default value is an empty string, is it a mistake?
There was a problem hiding this comment.
no, the type comes from the component and its 'error' | undefined so I just adjusted to match types from a component
| }} | ||
| disabled={didConfirm} | ||
| interactive={!isReadOnly} | ||
| brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isCreatedMissing(transaction ?? null) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined} |
There was a problem hiding this comment.
Same here,
The previous default value is an empty string, is it a mistake?
There was a problem hiding this comment.
…s-migration/MoneyRequestConfirmationList/component
|
|
||
| const receiptData = receiptPath && receiptFilename ? ReceiptUtils.getThumbnailAndImageURIs(transaction ?? null, receiptPath, receiptFilename) : null; | ||
| return ( | ||
| // @ts-expect-error TODO: Remove this once OptionsSelector (https://github.com/Expensify/App/issues/25125) is migrated to TypeScript. |
There was a problem hiding this comment.
Do we have a follow up issue for this?
…s-migration/MoneyRequestConfirmationList/component
|
@AndrewGable Yes we have, #25125 |
|
Thanks - FYI, There are now conflicts. |
…s-migration/MoneyRequestConfirmationList/component
|
@AndrewGable conflicts fixed 😄 |
…s-migration/MoneyRequestConfirmationList/component
…s-migration/MoneyRequestConfirmationList/component
|
@kubabutkiewicz heads up - lint is failing here |
…s-migration/MoneyRequestConfirmationList/component
|
@roryabraham thanks for a heads up, lint check is fixed now |
|
@hoangzinh - Do you mind running through the tests again once more? This is an important page to not break, so just want to make sure it's well tested before we merge it. |
|
@AndrewGable sure, I will try to test again today |
hoangzinh
left a comment
hoangzinh
left a comment
There was a problem hiding this comment.
I tested again. Looks good cc @AndrewGable
|
✋ 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.45-0 🚀
|
|
FYI this PR caused too many regressions and we are going to revert it. I'd be happy to review the next PR that addresses these issues. |
|
Will work on this tomorrow, sorry ! @AndrewGable |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.45-6 🚀
|
7 participants
Details
Fixed Issues
$ #25138
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
android.mp4
Android: mWeb Chrome
mchrome.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4