Skip to content
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

[HOLD for payment 2022-11-02] [$1000] [Form Refactor] AddressForm #10738

Closed
8 tasks
neil-marcellini opened this issue Sep 1, 2022 · 41 comments
Closed
8 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@neil-marcellini
Copy link
Contributor

Coming from the New Expensify Forms design doc, we should refactor the inputs within the AddressForm to be compatible with the new Form component, following the guidelines below:

Here's an example of a Form refactor: #9056
Here is a component that had its inputs refactored to be compatible with Form.js.

Guidelines

Contributors should first read FORMS.md.
The refactored inputs should be functional when wrapped by Form.js and when not.

  1. Add an optional inputID prop.
  2. Add an optional shouldSaveDraft prop that defaults to false.
  3. Make the value prop optional.
  4. Change any onChange prop, e.g. onTextChange to onChange.
  5. In the input’s onBlur method, call props.onBlur().
  6. In the input’s onChange method, call props.onChange() .
  7. Remove the hasError prop.
  8. Update the error message to display if errorText is truthy.
  9. Make sure that props.ref is attached to the appropriate DOM node. This could involve forwarding the ref to a child component.
  10. Make sure the ref is exposing both value and focus().
  11. Add an optional maxLength prop to text inputs and UI to display the current character count out of the limit.
  12. Add a hint prop and display the text under the input if there is no existing error.
  13. Remove any unused code.

Testing

Verify that:

  • UI looks as it did before the refactor
  • Values can be added and edited
  • Errors are highlighted correctly (input border)
  • Error messages show up correctly
  • Draft values are saved properly
  • Form alerts are displayed correctly
  • Clicking the fix the errors link focuses the first input with error
  • No duplicate submission of the form occurs (when it's already submitting)
@neil-marcellini neil-marcellini added External Added to denote the issue can be worked on by a contributor Engineering labels Sep 1, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 1, 2022

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot melvin-bot bot added the Daily KSv2 label Sep 1, 2022
@kakajann
Copy link
Contributor

kakajann commented Sep 1, 2022

May I work on this?
I've refactored AddressSearch before in here #7701

@neil-marcellini
Copy link
Contributor Author

I'm going to give priority to @mollfpr coming from the IdentityForm issue. If that doesn't work out for some reason then we will consider your proposal.

@mollfpr
Copy link
Contributor

mollfpr commented Sep 1, 2022

Proposal

  1. Adding onChange props to our input component
  2. Adding inputIDs props to AddressForm with a default value is an empty string for every input. These props will be used to determine what inputID is for the street, city, state, and zipCode.
  3. Adding shouldSaveDraft props to AddressForm with default false and will pass through all the inputs inside AddressForm.
  4. Adding onBlur props to AddressForm and passing it through the inputs.
  5. Change the use of onChangeText on TextInput to onChange.
diff --git a/src/components/TextInput/BaseTextInput.js b/src/components/TextInput/BaseTextInput.js
index df829f7fd..e1d5a0cfe 100644
--- a/src/components/TextInput/BaseTextInput.js
+++ b/src/components/TextInput/BaseTextInput.js
@@ -273,6 +273,7 @@ class BaseTextInput extends Component {
                                         maxLength={this.props.maxLength}
                                         onFocus={this.onFocus}
                                         onBlur={this.onBlur}
+                                        onChange={this.props.onChange}
                                         onChangeText={this.setValue}
                                         secureTextEntry={this.state.passwordHidden}
                                         onPressOut={this.props.onPress}
diff --git a/src/components/TextInput/baseTextInputPropTypes.js b/src/components/TextInput/baseTextInputPropTypes.js
index 930f162cf..cc389cedc 100644
--- a/src/components/TextInput/baseTextInputPropTypes.js
+++ b/src/components/TextInput/baseTextInputPropTypes.js
@@ -65,6 +65,9 @@ const propTypes = {
     /** Callback to update the value on Form when input is used in the Form component. */
     onInputChange: PropTypes.func,

+    /** Callback that is called when the text input's text changes */
+    onChange: PropTypes.func,
+
     /** Whether we should wait before focusing the TextInput, useful when using transitions  */
     shouldDelayFocus: PropTypes.bool,
 };
@@ -96,6 +99,7 @@ const defaultProps = {
     hint: '',
     prefixCharacter: '',
     onInputChange: () => {},
+    onChange: () => {},
     shouldDelayFocus: false,
 };
diff --git a/src/pages/ReimbursementAccount/AddressForm.js b/src/pages/ReimbursementAccount/AddressForm.js
index 41c8fa40c..3afbf0bca 100644
--- a/src/pages/ReimbursementAccount/AddressForm.js
+++ b/src/pages/ReimbursementAccount/AddressForm.js
@@ -16,6 +16,8 @@ const propTypes = {
     /** Callback fired when a field changes. Passes args as {[fieldName]: val} */
     onFieldChange: PropTypes.func.isRequired,

+    onBlur: PropTypes.func,
+
     /** Form values */
     values: PropTypes.shape({
         /** Address street field */
@@ -31,6 +33,22 @@ const propTypes = {
         zipCode: PropTypes.string,
     }),

+    inputIDs: PropTypes.shape({
+        /** Address street field */
+        street: PropTypes.string,
+
+        /** Address city field */
+        city: PropTypes.string,
+
+        /** Address state field */
+        state: PropTypes.string,
+
+        /** Address zip code field */
+        zipCode: PropTypes.string,
+    }),
+
+    shouldSaveDraft: PropTypes.bool,
+
     /** Any errors that can arise from form validation */
     errors: PropTypes.objectOf(PropTypes.bool),

@@ -44,44 +62,64 @@ const defaultProps = {
         state: '',
         zipCode: '',
     },
+    inputIDs: {
+        street: '',
+        city: '',
+        state: '',
+        zipCode: '',
+    },
+    shouldSaveDraft: false,
     errors: {},
+    onBlur: () => {},
 };

 const AddressForm = props => (
     <>
         <AddressSearch
+            inputID={props.inputIDs.street}
             label={props.translate(props.streetTranslationKey)}
             containerStyles={[styles.mt4]}
             value={props.values.street}
             onInputChange={props.onFieldChange}
+            onBlur={props.onBlur}
             errorText={props.errors.street ? props.translate('bankAccount.error.addressStreet') : ''}
             hint={props.translate('common.noPO')}
+            shouldSaveDraft={props.shouldSaveDraft}
         />
         <View style={[styles.flexRow, styles.mt4]}>
             <View style={[styles.flex2, styles.mr2]}>
                 <TextInput
+                    inputID={props.inputIDs.city}
                     label={props.translate('common.city')}
                     value={props.values.city}
-                    onChangeText={value => props.onFieldChange({city: value})}
+                    onChange={({nativeEvent}) => props.onFieldChange({city: nativeEvent.text})}
+                    onBlur={props.onBlur}
                     errorText={props.errors.city ? props.translate('bankAccount.error.addressCity') : ''}
+                    shouldSaveDraft={props.shouldSaveDraft}
                 />
             </View>
             <View style={[styles.flex1]}>
                 <StatePicker
+                    inputID={props.inputIDs.state}
                     value={props.values.state}
                     onInputChange={value => props.onFieldChange({state: value})}
+                    onBlur={props.onBlur}
                     errorText={props.errors.state ? props.translate('bankAccount.error.addressState') : ''}
+                    shouldSaveDraft={props.shouldSaveDraft}
                 />
             </View>
         </View>
         <TextInput
+            inputID={props.inputIDs.zipCode}
             label={props.translate('common.zip')}
             containerStyles={[styles.mt4]}
             keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD}
             value={props.values.zipCode}
-            onChangeText={value => props.onFieldChange({zipCode: value})}
+            onChange={({nativeEvent}) => props.onFieldChange({zipCode: nativeEvent.text})}
+            onBlur={props.onBlur}
             errorText={props.errors.zipCode ? props.translate('bankAccount.error.zipCode') : ''}
             maxLength={CONST.BANK_ACCOUNT.MAX_LENGTH.ZIP_CODE}
+            shouldSaveDraft={props.shouldSaveDraft}
         />
     </>
 );

cc @neil-marcellini

@mollfpr
Copy link
Contributor

mollfpr commented Sep 2, 2022

@neil-marcellini I have checked the Form.js, but it can't modify the element inside AddressForm. What I understand is in Form.js it will loop through the this.props.children element and try to find an element with an inputID and then modified the prop element. The element nested in AddressForm doesn't expose through the element from Form.js.

Any idea how Form.js can find the input element inside the AddressForm? I'm kinda stuck figuring it out.

@mollfpr
Copy link
Contributor

mollfpr commented Sep 4, 2022

I have to refactor Form.js to pass the props input inside AddressForm from the props (not directly return the element from Form.js).

diff --git a/src/components/Form.js b/src/components/Form.js
index f42101700..70073f9fb 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -63,6 +63,7 @@ class Form extends React.Component {
         this.inputValues = {};
         this.touchedInputs = {};

+        this.setPropsForInput = this.setPropsForInput.bind(this);
         this.setTouchedInput = this.setTouchedInput.bind(this);
         this.validate = this.validate.bind(this);
         this.submit = this.submit.bind(this);
@@ -75,6 +76,31 @@ class Form extends React.Component {
         this.touchedInputs[inputID] = true;
     }

+    setPropsForInput(inputID, defaultValue, shouldSaveDraft) {
+        return {
+            ref: node => this.inputRefs[inputID] = node,
+            defaultValue,
+            errorText: this.state.errors[inputID] || '',
+            onBlur: () => {
+                this.setTouchedInput(inputID);
+                this.validate(this.inputValues);
+            },
+            onInputChange: (value, key) => {
+                const inputKey = key || inputID;
+                this.inputValues[inputKey] = value;
+                const inputRef = this.inputRefs[inputKey];
+
+                if (key && inputRef && _.isFunction(inputRef.setNativeProps)) {
+                    inputRef.setNativeProps({value});
+                }
+                if (shouldSaveDraft) {
+                    FormActions.setDraftValues(this.props.formID, {[inputKey]: value});
+                }
+                this.validate(this.inputValues);
+            },
+        };
+    }
+
     submit() {
         // Return early if the form is already submitting to avoid duplicate submission
         if (this.props.formState.isLoading) {
@@ -127,6 +153,26 @@ class Form extends React.Component {
                 return child;
             }

+            if (child.props.inputIDs) {
+                const inputProps = {};
+
+                _.each(child.props.inputIDs, (inputID, key) => {
+                    const defaultValues = child.props.defaultValues || {};
+                    const defaultValue = this.props.draftValues[inputID] || defaultValues[key];
+
+                    // We want to initialize the input value if it's undefined
+                    if (_.isUndefined(this.inputValues[inputID])) {
+                        this.inputValues[inputID] = defaultValue;
+                    }
+
+                    inputProps[key] = this.setPropsForInput(inputID, defaultValue, child.props.shouldSaveDraft);
+                });
+
+                return React.cloneElement(child, {
+                    inputProps,
+                });
+            }
+
             // Depth first traversal of the render tree as the input element is likely to be the last node
             if (child.props.children) {
                 return React.cloneElement(child, {
@@ -149,28 +195,7 @@ class Form extends React.Component {
                 this.inputValues[inputID] = defaultValue;
             }

-            return React.cloneElement(child, {
-                ref: node => this.inputRefs[inputID] = node,
-                defaultValue,
-                errorText: this.state.errors[inputID] || '',
-                onBlur: () => {
-                    this.setTouchedInput(inputID);
-                    this.validate(this.inputValues);
-                },
-                onInputChange: (value, key) => {
-                    const inputKey = key || inputID;
-                    this.inputValues[inputKey] = value;
-                    const inputRef = this.inputRefs[inputKey];
-
-                    if (key && inputRef && _.isFunction(inputRef.setNativeProps)) {
-                        inputRef.setNativeProps({value});
-                    }
-                    if (child.props.shouldSaveDraft) {
-                        FormActions.setDraftValues(this.props.formID, {[inputKey]: value});
-                    }
-                    this.validate(this.inputValues);
-                },
-            });
+            return React.cloneElement(child, this.setPropsForInput(inputID, defaultValue, child.props.shouldSaveDraft));
         });
     }
diff --git a/src/pages/ReimbursementAccount/AddressForm.js b/src/pages/ReimbursementAccount/AddressForm.js
index 41c8fa40c..29110eebd 100644
--- a/src/pages/ReimbursementAccount/AddressForm.js
+++ b/src/pages/ReimbursementAccount/AddressForm.js
@@ -8,13 +8,21 @@ import withLocalize, {withLocalizePropTypes} from '../../components/withLocalize
 import CONST from '../../CONST';
 import StatePicker from '../../components/StatePicker';

+const inputPropsType = PropTypes.shape({
+    ref: PropTypes.func,
+    defaultValue: PropTypes.string,
+    errorText: PropTypes.string,
+    onBlur: PropTypes.func,
+    onInputChange: PropTypes.func,
+});
+
 const propTypes = {

     /** Translate key for Street name */
     streetTranslationKey: PropTypes.string.isRequired,

     /** Callback fired when a field changes. Passes args as {[fieldName]: val} */
-    onFieldChange: PropTypes.func.isRequired,
+    onFieldChange: PropTypes.func,

     /** Form values */
     values: PropTypes.shape({
@@ -34,54 +42,117 @@ const propTypes = {
     /** Any errors that can arise from form validation */
     errors: PropTypes.objectOf(PropTypes.bool),

+    inputIDs: PropTypes.shape({
+        /** Address street field */
+        street: PropTypes.string,
+
+        /** Address city field */
+        city: PropTypes.string,
+
+        /** Address state field */
+        state: PropTypes.string,
+
+        /** Address zip code field */
+        zipCode: PropTypes.string,
+    }),
+
+    inputProps: PropTypes.shape({
+        /** Address street field */
+        street: inputPropsType,
+
+        /** Address city field */
+        city: inputPropsType,
+
+        /** Address state field */
+        state: inputPropsType,
+
+        /** Address zip code field */
+        zipCode: inputPropsType,
+    }),
+
+    shouldSaveDraft: PropTypes.bool,
+
     ...withLocalizePropTypes,
 };

 const defaultProps = {
+    onFieldChange: () => {},
     values: {
         street: '',
         city: '',
         state: '',
         zipCode: '',
     },
+    inputIDs: {
+        street: '',
+        city: '',
+        state: '',
+        zipCode: '',
+    },
+    inputProps: {
+        street: {},
+        city: {},
+        state: {},
+        zipCode: {},
+    },
     errors: {},
+    shouldSaveDraft: false,
 };

 const AddressForm = props => (
     <>
-        <AddressSearch
-            label={props.translate(props.streetTranslationKey)}
-            containerStyles={[styles.mt4]}
-            value={props.values.street}
-            onInputChange={props.onFieldChange}
-            errorText={props.errors.street ? props.translate('bankAccount.error.addressStreet') : ''}
-            hint={props.translate('common.noPO')}
-        />
+        <View>
+            <AddressSearch
+                inputID={props.inputIDs.street}
+                shouldSaveDraft={props.shouldSaveDraft}
+                label={props.translate(props.streetTranslationKey)}
+                containerStyles={[styles.mt4]}
+                value={props.values.street || undefined}
+                onInputChange={props.onFieldChange}
+                errorText={props.errors.street ? props.translate('bankAccount.error.addressStreet') : ''}
+                hint={props.translate('common.noPO')}
+                renamedInputKeys={props.inputIDs}
+                // eslint-disable-next-line react/jsx-props-no-spreading
+                {...props.inputProps.street}
+            />
+        </View>
         <View style={[styles.flexRow, styles.mt4]}>
             <View style={[styles.flex2, styles.mr2]}>
                 <TextInput
+                    inputID={props.inputIDs.city}
+                    shouldSaveDraft={props.shouldSaveDraft}
                     label={props.translate('common.city')}
-                    value={props.values.city}
+                    value={props.values.city || undefined}
                     onChangeText={value => props.onFieldChange({city: value})}
                     errorText={props.errors.city ? props.translate('bankAccount.error.addressCity') : ''}
+                    // eslint-disable-next-line react/jsx-props-no-spreading
+                    {...props.inputProps.city}
                 />
             </View>
             <View style={[styles.flex1]}>
                 <StatePicker
-                    value={props.values.state}
+                    inputID={props.inputIDs.state}
+                    shouldSaveDraft={props.shouldSaveDraft}
+                    value={props.values.state || undefined}
                     onInputChange={value => props.onFieldChange({state: value})}
                     errorText={props.errors.state ? props.translate('bankAccount.error.addressState') : ''}
+                    // eslint-disable-next-line react/jsx-props-no-spreading
+                    {...props.inputProps.state}
                 />
             </View>
         </View>
         <TextInput
+            inputID={props.inputIDs.zipCode}
+            shouldSaveDraft={props.shouldSaveDraft}
             label={props.translate('common.zip')}
             containerStyles={[styles.mt4]}
             keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD}
-            value={props.values.zipCode}
+            value={props.values.zipCode || undefined}
             onChangeText={value => props.onFieldChange({zipCode: value})}
             errorText={props.errors.zipCode ? props.translate('bankAccount.error.zipCode') : ''}
             maxLength={CONST.BANK_ACCOUNT.MAX_LENGTH.ZIP_CODE}
+            // eslint-disable-next-line react/jsx-props-no-spreading
+            {...props.inputProps.zipCode}
         />
     </>
 );

The usage of AddressForm inside Form.js will be like this:

                    <AddressForm
                        inputIDs={{
                            street: 'addressStreet',
                            city: 'addressCity',
                            zipCode: 'addressZipCode',
                            state: 'addressState',
                        }}
                        streetTranslationKey="common.companyAddress"
                        shouldSaveDraft
                    />

cc @neil-marcellini

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2022

@puneetlath Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (Exported)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 6, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2022

Current assignee @puneetlath is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title [Form Refactor] AddressForm [$250] [Form Refactor] AddressForm Sep 6, 2022
@puneetlath
Copy link
Contributor

@rushatgabhane
Copy link
Member

@puneetlath sorry, I don't have the bandwidth

Can you please reassign by re-adding the Exported Label

@rushatgabhane rushatgabhane removed their assignment Sep 6, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (Exported)

@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2022

Current assignee @puneetlath is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot removed the Overdue label Oct 3, 2022
@puneetlath
Copy link
Contributor

Sorry for being so slow on this one. I'm good with it to. Let's do it @mollfpr 💪🏾

@melvin-bot melvin-bot bot removed Help Wanted Apply this label when an issue is open to proposals by contributors Daily KSv2 labels Oct 4, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2022

📣 @mollfpr You have been assigned to this job by @puneetlath!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@mananjadhav
Copy link
Collaborator

PR is reviewed and approved from my side. Should be close to merge soon.

@puneetlath @luacmartins Quick bump on the PR.

@melvin-bot melvin-bot bot removed the Overdue label Oct 19, 2022
@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@luacmartins luacmartins added NewFeature Something to build that is a new item. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Oct 24, 2022
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 26, 2022
@melvin-bot melvin-bot bot changed the title [$1000] [Form Refactor] AddressForm [HOLD for payment 2022-11-02] [$1000] [Form Refactor] AddressForm Oct 26, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 26, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.19-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-11-02. 🎊

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Nov 2, 2022
@puneetlath
Copy link
Contributor

@mollfpr @mananjadhav the old Upwork job got closed. Can y'all apply to this one so I can get you paid out? Comment here when you have. Thanks!

https://www.upwork.com/jobs/~015ac0b1950f970f4d

@melvin-bot melvin-bot bot removed the Overdue label Nov 2, 2022
@mollfpr
Copy link
Contributor

mollfpr commented Nov 2, 2022

@puneetlath Applied, thank you!

@mananjadhav
Copy link
Collaborator

@puneetlath applied

@puneetlath
Copy link
Contributor

All paid. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
None yet
Development

No branches or pull requests

9 participants