[TS migration] Migrate WorkspaceReimburse Page#35399
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
There was a problem hiding this comment.
| if (onInputChange) { | |
| onInputChange(inputValue); | |
| } | |
| onInputChange?.(inputValue); |
There was a problem hiding this comment.
| if (onInputChange) { | |
| onInputChange(inputValue, index); | |
| } | |
| onInputChange?.(inputValue, index); |
There was a problem hiding this comment.
| const getUnitLabel = useCallback((value: Unit) => translate(`common.${value}`), [translate]); | |
| const getUnitLabel = useCallback((value: Unit): string => translate(`common.${value}`), [translate]); |
There was a problem hiding this comment.
| const [currentRatePerUnit, setCurrentRatePerUnit] = useState<string | undefined>(''); | |
| const [currentRatePerUnit, setCurrentRatePerUnit] = useState<string>(''); |
There was a problem hiding this comment.
Let's update getUnitRateValue typing instead, it looks like it already can handle the case where customUnitRate is undefined
| if (!customUnitRate) { | |
| return undefined; | |
| } |
There was a problem hiding this comment.
Also maybe I'd rename it to NewCustomUnit or CustomUnitUpdate
| type UpdateCustomUnit = { | |
| customUnitID: string; | |
| name: string; | |
| attributes: Attributes; | |
| rates: Rate; | |
| }; | |
| type UpdateCustomUnit = CustomUnit & { | |
| rates: Rate; | |
| }; |
There was a problem hiding this comment.
Same, update getUnitRateValue typing instead
There was a problem hiding this comment.
| <View style={[styles.mt4]}> | |
| <View style={styles.mt4}> |
There was a problem hiding this comment.
Can we extract this url to CONST?
There was a problem hiding this comment.
Do you mean add this into the CONST file?
6dcbdc6 to
4feb092
Compare
| }).isRequired, | ||
|
|
||
| type WorkspaceReimburseViewOnyxProps = { | ||
| /** From Onyx */ |
There was a problem hiding this comment.
| /** From Onyx */ |
| }, | ||
| }; | ||
|
|
||
| Policy.updateWorkspaceCustomUnitAndRate(policy?.id ?? '', distanceCustomUnit, newCustomUnit, parseInt(policy?.lastModified ?? '', 2)); |
There was a problem hiding this comment.
Please update updateWorkspaceCustomUnitAndRate typing instead
| Policy.updateWorkspaceCustomUnitAndRate(policy?.id ?? '', distanceCustomUnit, newCustomUnit, parseInt(policy?.lastModified ?? '', 2)); | |
| Policy.updateWorkspaceCustomUnitAndRate(policy?.id ?? '', distanceCustomUnit, newCustomUnit, policy?.lastModified); |
| if (!rateValueRegex.test(values.rate.toString()) || values.rate.toString() === 'Nan') { | ||
| errors.rate = 'workspace.reimburse.invalidRateError'; | ||
| } else if (NumberUtils.parseFloatAnyLocale(values.rate.toString()) <= 0) { | ||
| errors.rate = 'workspace.reimburse.lowRateError'; | ||
| } |
There was a problem hiding this comment.
Let's have the old logic and update rate type in values to string instead
| if (!rateValueRegex.test(values.rate.toString()) || values.rate.toString() === 'Nan') { | |
| errors.rate = 'workspace.reimburse.invalidRateError'; | |
| } else if (NumberUtils.parseFloatAnyLocale(values.rate.toString()) <= 0) { | |
| errors.rate = 'workspace.reimburse.lowRateError'; | |
| } | |
| if (!rateValueRegex.test(values.rate) || values.rate === '') { | |
| errors.rate = 'workspace.reimburse.invalidRateError'; | |
| } else if (NumberUtils.parseFloatAnyLocale(values.rate) <= 0) { | |
| errors.rate = 'workspace.reimburse.lowRateError'; | |
| } |
# Conflicts: # src/ONYXKEYS.ts # src/pages/workspace/reimburse/WorkspaceRateAndUnitPage.js # src/pages/workspace/reimburse/WorkspaceReimbursePage.js # src/pages/workspace/reimburse/WorkspaceReimburseView.tsx # src/types/onyx/index.ts
# Conflicts: # src/ONYXKEYS.ts # src/libs/DistanceRequestUtils.ts # src/libs/actions/Policy.ts # src/pages/workspace/reimburse/WorkspaceReimburseView.tsx # src/types/onyx/Form.ts
| type UpdateWorkspaceCustomUnitAndRateParams = { | ||
| policyID: string; | ||
| lastModified: number; | ||
| lastModified?: number | string; |
There was a problem hiding this comment.
| lastModified?: number | string; | |
| lastModified?: string; |
| import type {Unit} from './Policy'; | ||
|
|
||
| type FormValueType = string | boolean | Date | OnyxCommon.Errors; | ||
| type FormValueType = string | boolean | Date | number | OnyxCommon.Errors; |
There was a problem hiding this comment.
Why number it added, I've tried to remove it and ts check pass
| <OfflineWithFeedback | ||
| pendingAction={distanceCustomUnit?.pendingAction ?? distanceCustomRate?.pendingAction} | ||
| shouldShowErrorMessages={false} | ||
| > | ||
| <MenuItemWithTopDescription | ||
| title={currentRatePerUnit} | ||
| description={translate('workspace.reimburse.trackDistanceRate')} | ||
| shouldShowRightIcon | ||
| onPress={() => Navigation.navigate(ROUTES.WORKSPACE_RATE_AND_UNIT.getRoute(policy?.id ?? ''))} | ||
| wrapperStyle={[styles.mhn5, styles.wAuto]} | ||
| brickRoadIndicator={(distanceCustomUnit?.errors ?? distanceCustomRate?.errors) && CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR} | ||
| /> | ||
| </OfflineWithFeedback> |
There was a problem hiding this comment.
Why was it added, the code looks really similar to the code block above, maybe you did a mistake during conflicts resolution?
There was a problem hiding this comment.
You are right! It was badly conflicts merge! Thank you!
| const currentCustomUnitRate = Object.values(distanceCustomUnit?.rates ?? {}).find((r) => r.name === CONST.CUSTOM_UNITS.DEFAULT_RATE); | ||
| const unitID = distanceCustomUnit.customUnitID ?? ''; | ||
| const unitName = distanceCustomUnit.name ?? ''; | ||
| const rateNumValue = PolicyUtils.getNumericValue(rate, toLocaleDigit) as number; |
There was a problem hiding this comment.
What if we use Number(rateNumValue) instead of assertion?
| const rateNumValue = PolicyUtils.getNumericValue(rate, toLocaleDigit) as number; | |
| const rateNumValue = PolicyUtils.getNumericValue(rate, toLocaleDigit); |
| attributes: {unit}, | ||
| rates: { | ||
| ...currentCustomUnitRate, | ||
| rate: rateNumValue * CONST.POLICY.CUSTOM_UNIT_RATE_BASE_OFFSET, |
There was a problem hiding this comment.
| rate: rateNumValue * CONST.POLICY.CUSTOM_UNIT_RATE_BASE_OFFSET, | |
| rate: Number(rateNumValue) * CONST.POLICY.CUSTOM_UNIT_RATE_BASE_OFFSET, |
|
Will get to this one tomorrow. |
|
@ruben-rebelo Could you have a look at those merge conflicts? Thanks! |
|
@ruben-rebelo Seems there are some lint/typecheck errors. |
da439a9 to
34135a2
Compare
# Conflicts: # src/ONYXKEYS.ts # src/libs/PolicyUtils.ts # src/libs/actions/Policy.ts # src/pages/workspace/reimburse/WorkspaceReimburseView.tsx # src/types/onyx/Form.ts # src/types/onyx/Policy.ts # src/types/onyx/index.ts
|
@Ollyws It seems the conflicts were wrongly resolved. |
|
@ruben-rebelo Still got a typecheck error there if you could take a look, thanks! |
Reviewer Checklist
Screenshots/VideosAndroid: Native01_Android_Native.mp4Android: mWeb Chrome02_Android_Chrome.mp4iOS: Native03_iOS_Native.mp4iOS: mWeb Safari04_iOS_Safari.mp4MacOS: Chrome / Safari05_MacOS_Chrome.mp4MacOS: Desktop06_MacOS_Desktop.mp4 |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25211 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@ruben-rebelo could you please run |
# Conflicts: # src/pages/workspace/reimburse/WorkspaceReimburseSection.tsx
|
@Ollyws Friendly bump |
|
Just waiting on @tylerkaraszewski's review. |
# Conflicts: # src/libs/DistanceRequestUtils.ts
|
Conflicts. |
# Conflicts: # src/libs/PolicyUtils.ts
|
@tylerkaraszewski Conflicts resolved. Thanks! |
|
Tests are currently failing. |
|
Also, conflicts. |
# Conflicts: # src/libs/PolicyUtils.ts
|
@tylerkaraszewski PR updated and conflicts resolved |
|
Performance tests aren't currently passing. |
|
@ruben-rebelo Could you check this? Thanks! |
|
@tylerkaraszewski It needed merge with main. Tests are now passing. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Details
[TS migration] Migrate WorkspaceReimburse page to TypeScript
Fixed Issues
$ #25211
PROPOSAL: N/A
Tests
Test WorkspaceReimburse Pages:
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
android-native.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios-native.mp4
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
macos-web.mp4
MacOS: Desktop
macos-desktop.mov