Work with Form.js wrapper and refactor address form #11611
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
mollfpr
changed the title
…orm-js-and-address-form
…orm-js-and-address-form
melvin-bot
bot
requested review from
mananjadhav and
puneetlath
and removed request for
a team
There was a problem hiding this comment.
Few comments and questions, @mollfpr. Can you help address them?
| }; | ||
|
|
||
| const defaultProps = { | ||
| values: { | ||
| street: undefined, |
There was a problem hiding this comment.
Any specific reason we're setting them as undefined and not '' ?
There was a problem hiding this comment.
App/src/components/TextInput/BaseTextInput.js
Lines 73 to 81 in 2f7f5ea
We need to pass
undefined to value if we use inputID, the value will be controlled on Form.js.
| @@ -14,7 +13,7 @@ const propTypes = { | |||
| streetTranslationKey: PropTypes.string.isRequired, | |||
|
|
|||
| /** Callback fired when a field changes. Passes args as {[fieldName]: val} */ | |||
| onFieldChange: PropTypes.func.isRequired, | |||
| onFieldChange: PropTypes.func, | |||
There was a problem hiding this comment.
We're removing isRequired because?
There was a problem hiding this comment.
Since the goal of these refactors is to make AddressForm usable with Form.js we don't need onFieldChange but it's kept to accommodate forms that do not use Form.js.
src/components/Form.js
Outdated
| if (_.isFunction(child.type)) { | ||
| const nestedChildren = new child.type(child.props); | ||
|
|
||
| if (!React.isValidElement(nestedChildren) || !nestedChildren.props.children) { |
There was a problem hiding this comment.
Are we always sure we'll have nestedChildren.props? I am wondering if this needs to be replaced with lodash.get ?
There was a problem hiding this comment.
Good thinking! I'm not really sure but let's use lodash.get instead.
| onInputChange={props.onFieldChange} | ||
| errorText={props.errors.street ? props.translate('bankAccount.error.addressStreet') : ''} | ||
| hint={props.translate('common.noPO')} | ||
| renamedInputKeys={props.inputKeys} |
There was a problem hiding this comment.
We're setting these empty because?
There was a problem hiding this comment.
Which one do you mean?
|
For testing the diff --git a/src/pages/ReimbursementAccount/CompanyStep.js b/src/pages/ReimbursementAccount/CompanyStep.js
index ac911276f..a8e6ae620 100644
--- a/src/pages/ReimbursementAccount/CompanyStep.js
+++ b/src/pages/ReimbursementAccount/CompanyStep.js
@@ -27,6 +27,7 @@ import ReimbursementAccountForm from './ReimbursementAccountForm';
import * as ReimbursementAccount from '../../libs/actions/ReimbursementAccount';
import * as ReimbursementAccountUtils from '../../libs/ReimbursementAccountUtils';
import * as FormActions from '../../libs/actions/FormActions';
+import Form from '../../components/Form';
const propTypes = {
...withLocalizePropTypes,
@@ -188,7 +189,32 @@ class CompanyStep extends React.Component {
onCloseButtonPress={Navigation.dismissModal}
/>
- <ReimbursementAccountForm onSubmit={this.submit}>
+ <Form
+ formID="testForm"
+ submitButtonText="Submit"
+ validate={(values) => {
+ const errors = {};
+
+ if (!ValidationUtils.isRequiredFulfilled(values.addressStreet)) {
+ errors.addressStreet = this.props.translate('bankAccount.error.addressStreet');
+ }
+
+ if (!ValidationUtils.isRequiredFulfilled(values.addressCity)) {
+ errors.addressCity = this.props.translate('bankAccount.error.addressCity');
+ }
+
+ if (!ValidationUtils.isRequiredFulfilled(values.addressState)) {
+ errors.addressState = this.props.translate('bankAccount.error.addressState');
+ }
+
+ if (!ValidationUtils.isRequiredFulfilled(values.addressZipCode)) {
+ errors.addressZipCode = this.props.translate('bankAccount.error.zipCode');
+ }
+
+ return errors;
+ }}
+ onSubmit={this.submit}
+ >
<Text>{this.props.translate('companyStep.subtitle')}</Text>
<TextInput
label={this.props.translate('companyStep.legalBusinessName')}
@@ -199,31 +225,14 @@ class CompanyStep extends React.Component {
errorText={this.getErrorText('companyName')}
/>
<AddressForm
+ shouldSaveDraft
translate={this.props.translate}
streetTranslationKey="common.companyAddress"
- values={{
- street: this.state.addressStreet,
- city: this.state.addressCity,
- zipCode: this.state.addressZipCode,
- state: this.state.addressState,
- }}
- errors={{
- street: this.getErrors().addressStreet,
- city: this.getErrors().addressCity,
- zipCode: this.getErrors().addressZipCode,
- state: this.getErrors().addressState,
- }}
- onFieldChange={(values) => {
- const renamedFields = {
- street: 'addressStreet',
- state: 'addressState',
- city: 'addressCity',
- zipCode: 'addressZipCode',
- };
- _.each(values, (value, inputKey) => {
- const renamedInputKey = lodashGet(renamedFields, inputKey, inputKey);
- this.clearErrorAndSetValue(renamedInputKey, value);
- });
+ inputKeys={{
+ street: 'addressStreet',
+ city: 'addressCity',
+ state: 'addressState',
+ zipCode: 'addressZipCode',
}}
/>
<TextInput
@@ -297,7 +306,7 @@ class CompanyStep extends React.Component {
style={[styles.mt4]}
errorText={this.getErrorText('hasNoConnectionToCannabis')}
/>
- </ReimbursementAccountForm>
+ </Form>
</>
);
}Screen.Recording.2022-10-11.at.22.21.15.mov |
|
Thanks for the comment @mollfpr. Code looks fine now, I'll test and update my feedback here by tomorrow! |
There was a problem hiding this comment.
Thanks for the PR @mollfpr. Apologies it took a bit longer, as needed to ensure that current flow doesn't break as well as it should work with new Form.js.
Just to keep everyone updated, we are only refactoring the AddressForm to make it compatible with Form.js. It should be backward compatible, and the other forms will be using this refactored component such as #9580
@puneetlath All yours. Can we also tag @luacmartins as he was involved in the solutioning and would be great to have another pair of eyes for the review.
🎀 👀 🎀
C+ reviewed
- I have verified the author checklist is complete (all boxes are checked off).
- I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I verified the steps for local testing are in the
- I checked that screenshots or videos are included for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
- If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
- I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
toggleReportand notonIconClick). - I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all
src/languages/*files - I verified any copy / text that was added to the app is correct English and approved by marketing by adding the
Waiting for Copylabel for a copy review on the original GH to get the correct copy. - I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in
STYLE.md) were followed
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
- If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarhave been tested & I retested again) - I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
- I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
- If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a
/** comment above it */ - Any functional components have the
displayNameproperty - The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
- If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e.
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
- If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avataris modified, I verified thatAvataris working as expected in all cases) - If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
- I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.
|
@mollfpr The check list is failing. Can you confirm if it is inline with the latest template? @puneetlath I guess the checklist action isn't working fine. |
…orm-js-and-address-form
|
@mananjadhav Thank you for the review! I just pulled the main branch and updated the author checklist. The checklist running well now. |
|
Thanks @mollfpr. @puneetlath @luacmartins I am wondering if we need to update any documents regarding Form with a note about HOC |
@mananjadhav @mollfpr yes, let's update FORMS.md to mention that children component's can't be wrapped in HOC |
|
@mananjadhav @luacmartins Updated |
contributingGuides/FORMS.md
Outdated
| @@ -198,6 +198,38 @@ function onSubmit(values) { | |||
| </Form> | |||
| ``` | |||
|
|
|||
| `Form.js` also works with inputs component that's nested on a component that doesn't wrap with any HOC. | |||
There was a problem hiding this comment.
Can we simplify this statement? Even break it into two lines if required.
Form.js also works with inputs from a nested component. The only exception is that the nested component shouldn't be wrapped around any HoC.
@luacmartins can also share thoughts on this one.
There was a problem hiding this comment.
Form.js also works with inputs from a nested component. The only exception is that the nested component shouldn't be wrapped around any HoC.
I'm onboard with your suggestion.
There was a problem hiding this comment.
I'd suggest:
Form.js also works with inputs nested in custom components, e.g. AddressForm. However, the custom component cannot be wrapped in any HOC.
|
@mananjadhav could you please update your checklist to the latest version (found here) and make sure that you upload videos for all platforms? |
contributingGuides/FORMS.md
Outdated
| @@ -198,6 +198,38 @@ function onSubmit(values) { | |||
| </Form> | |||
| ``` | |||
|
|
|||
| `Form.js` also works with inputs component that's nested on a component that doesn't wrap with any HOC. | |||
There was a problem hiding this comment.
I'd suggest:
Form.js also works with inputs nested in custom components, e.g. AddressForm. However, the custom component cannot be wrapped in any HOC.
Co-authored-by: Carlos Martins <luacmartins@gmail.com>
Co-authored-by: Carlos Martins <luacmartins@gmail.com>
|
@mananjadhav can you please update your reviewer checklist to the latest version and make sure that you add videos for all platforms please? |
|
Approved on my end. @mananjadhav bump me for a merge once you've updated the PR reviewer checklist as requested here. |
|
Yes I am on it. This one's on my todo today. |
|
@luacmartins @puneetlath Here's the updated checklist with the screenshots. @luacmartins The github action still shows it is incomplete.I a not sure why.
ScreenshotsWebweb-address-form.movMobile Web - Chromemweb-android-address-form.movMobile Web - Safarimweb-safari-address-form.movDesktopdesktop-address-form.moviOSios-address-form.movAndroidandroid-address-form.mov |
|
It's passing now. Just had to re-trigger it. |
|
@luacmartins looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the |
|
Tests passed. Removing |
|
✋ 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 @luacmartins in version: 1.2.19-0 🚀
|
|
🚀 Deployed to production by @chiragsalian in version: 1.2.19-2 🚀
|
Details
Form.jsto be able drill down the inputs inside another component fileAddressFormwithLocalizeHOC and use the passedtranslateprops from the parent componentinputKeysandshouldSaveDraftforForm.jsusageinputKeysaccording to the key name andshouldSaveDraftforForm.jsusageAddressFormto passtranslatepropsFixed Issues
$ #10738
Tests
AddressFormfor exampleCompanyStepformPR Review Checklist
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesWaiting 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)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
AddressFormfor exampleCompanyStepformScreenshots
Web
Refactor.AddressForm.-.Web.mov
Mobile Web - Chrome
5BRefactor.20AddressForm.5D.20-.20mWeb-Chrome.mov
Mobile Web - Safari
5BRefactor.20AddressForm.5D.20-.20mWeb-Safari.mov
Desktop
Refactor.AddressForm.-.Desktop.mov
iOS
Refactor.AddressForm.-.iOS.mov
Android
5BRefactor.20AddressForm.5D.20-.20Android.mov