-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TS migration] Migrate 'AddressForm.js' component to TypeScript #34149
[TS migration] Migrate 'AddressForm.js' component to TypeScript #34149
Conversation
c533366
to
4e88a52
Compare
610d212
to
70699c4
Compare
src/components/AddressForm.tsx
Outdated
city?: string; | ||
country?: string; | ||
state?: string; | ||
zipPostCode?: string | Array<string | Record<string, string>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zipPostCode?: string | Array<string | Record<string, string>>; | |
zipPostCode?: Localize.MaybePhraseKey; |
Could you test if this works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
70699c4
to
e4d33ce
Compare
|
||
// Check "State" dropdown is a valid state if selected Country is USA | ||
if (values.country === CONST.COUNTRY.US && !COMMON_CONST.STATES[values.state]) { | ||
if (values.country === CONST.COUNTRY.US && !values.state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does not work as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works as it was before since this value can be entered only with dropdown
src/components/AddressForm.tsx
Outdated
} | ||
|
||
if ('samples' in countryRegexDetails) { | ||
countryZipFormat = countryRegexDetails.samples as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are this assertions needed? What are the types of countryRegexDetails.regex
and countryRegexDetails.samples
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To specify types in this case (some countries don't have those fields and ts resolve those fields as unknown)
RegExp and string accordingly
src/types/onyx/Form.ts
Outdated
@@ -21,6 +23,35 @@ type DateOfBirthForm = Form & { | |||
dob?: string; | |||
}; | |||
|
|||
type AddressForm = Form & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has changed last week, please merge the newest main
src/components/AddressForm.tsx
Outdated
@@ -116,6 +144,7 @@ function AddressForm({city, country, formID, onAddressChanged, onSubmit, shouldS | |||
}, []); | |||
|
|||
return ( | |||
// @ts-expect-error TODO: Remove this once FormProvider (https://github.com/Expensify/App/issues/25109) is migrated to TypeScript. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge the newest main and follow new Form practices (check other forms with validate/onSubmit functions)
7bb77c3
to
5beeb4c
Compare
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@pasyukevich Could you please merge main? Thank you. |
@fedirjh PR updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We did not find an internal engineer to review this PR, trying to assign a random engineer to #31957 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
conflicts! |
b10b3c4
to
b45851b
Compare
@jasperhuangg PR updated |
@jasperhuangg looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ 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/jasperhuangg in version: 1.4.47-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.4.47-10 🚀
|
|
||
if (countrySpecificZipRegex) { | ||
if (countrySpecificZipRegex && values.zipPostCode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change broke the "is required" check for the zip codes. We shouldn't have added this condition and used the conditional chaining where needed instead.
Details
Fixed Issues
$ #31957
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android-native-converted.webm
Android: mWeb Chrome
android-web-converted.webm
iOS: Native
ios-native-converted.mp4
iOS: mWeb Safari
ios-web-converted.mp4
MacOS: Chrome / Safari
desktop-web-converted.mov
MacOS: Desktop
desktop-native-converted.mov