Refactor requestor step form #12766
Refactor requestor step form #12766
Conversation
melvin-bot
bot
requested review from
neil-marcellini and
Santhosh-Sellavel
and removed request for
a team
|
@neil-marcellini @Santhosh-Sellavel One of you needs to 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] |
| <Text | ||
| onPress={() => Link.openExternalLink('https://onfido.com/privacy/')} | ||
| style={[styles.textMicro, styles.link]} | ||
| accessibilityRole="link" | ||
| > | ||
| <Text onPress={() => Link.openExternalLink('https://onfido.com/privacy/')} style={[styles.textMicro, styles.link]} accessibilityRole="link"> | ||
| {`${this.props.translate('common.privacyPolicy')}`} | ||
| </Text> | ||
| {` ${this.props.translate('common.and')} `} | ||
| <Text | ||
| onPress={() => Link.openExternalLink('https://onfido.com/terms-of-service/')} | ||
| style={[styles.textMicro, styles.link]} | ||
| accessibilityRole="link" | ||
| > | ||
| <Text onPress={() => Link.openExternalLink('https://onfido.com/terms-of-service/')} style={[styles.textMicro, styles.link]} accessibilityRole="link"> |
There was a problem hiding this comment.
This seems like unnecessary
There was a problem hiding this comment.
Fix the unnecessary format changes
Reviewer Checklist
Screenshots/VideosDesktop & WebScreen.Recording.2022-11-17.at.9.31.57.PM.movMobile Web - ChromeAndroid_mWEb.movMobile Web - SafariiOS.mWEb.moviOS & AndroidScreen.Recording.2022-11-17.at.9.40.11.PM.mov |
|
@mollfpr Can you update the test steps more clearly, can add steps on how to navigate to
And these could be listed as a checklist Ex:
|
|
Updated test steps! @Santhosh-Sellavel |
|
I noticed the following issues while testing WarningSame one I found here, forgot to report in slack will do this Date Picker is not resettable (Not relevant to this issue)
Date Picker icon is not centered in Desktop & Web
State Picker Focus is not visible/conveyed to the user (Accessibility Issue)In the error view, clicking |
There was a problem hiding this comment.
@neil-marcellini Looks good to me.
I found a few issues, nothing was introduced here, so approving this one.
|
Also, let me know if should I raise those issues in slack and which issues if you want to ignore any let me know, thanks! |
|
I'm reviewing now 👀. I may be a bit slow since I want to be cautious. |
|
|
||
| // Prepare inputKeys for clearing errors | ||
| const inputKeys = _.keys(values); | ||
| if (!values.firstName) { |
There was a problem hiding this comment.
We need to trim the value first. Right now I can enter a space as my first name and it works. I didn't catch this in the proposal review but I noticed this because previously we used ValidationUtils.isRequiredFulfilled here.
App/src/libs/ValidationUtils.js
Line 226 in 368aa35
We should probably use that method. The same problem occurs for the last name, street, city, and maybe others.
Screen.Recording.2022-11-17.at.10.43.19.AM.mov
|
@Santhosh-Sellavel all of the bugs you mentioned here #12766 (comment) also exist on staging or prod correct? If so please report them. |
|
Updated PR @Santhosh-Sellavel @neil-marcellini |
Tests
After
this-pr.movStaging staging.mov
|
|
@neil-marcellini Yeah, I notice that too I put a heads-up in here #9581 (comment) Should we put the fix in this PR or log it into another issue? |
If this is not introduced here, we should log an issue. If not fix it here! |
|
It's reproducible on staging. Go to Settings > Payments > Debit card and type a text in the billing address input and select the option. @neil-marcellini @Santhosh-Sellavel Screen.Recording.2022-11-21.at.22.22.48.mov |
Check you on the same form we are refactoring here! |
|
Okay, I found the cause of diff --git a/src/components/Form.js b/src/components/Form.js
index b7b5f72ed..14f46e9ca 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -78,6 +78,14 @@ class Form extends React.Component {
this.submit = this.submit.bind(this);
}
+ shouldComponentUpdate(nextProps, nextState) {
+ if (_.isEqual(nextState.inputValues, this.state.inputValues) && _.isEqual(nextState.errors, this.state.errors)) {
+ return false;
+ }
+
+ return true;
+ }
+
/**
* @param {String} inputID - The inputID of the input being touched
*/Screen.Recording.2022-11-21.at.23.15.52.mov |
|
@mollfpr Thanks for doing some investigation. Even though the same error exists on staging, it is on another form, so I don't want to introduce another regression in another place. Let's fix it in this PR.
Why is the focus moved to the body before clicking the option? Is that the intended behavior? What happens on staging? Are you sure there won't be any problems caused by preventing the form component from updating if values and errors stay the same? |
When clicking the option, the
Is not.
Same as here, in
I believe is not, because the input is changed only due to the input value change or there's an error. The simple solution we can do is when validate function is called, we check if the diff --git a/src/components/Form.js b/src/components/Form.js
index b7b5f72ed..5bdf994ba 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -125,7 +125,11 @@ class Form extends React.Component {
const errors = _.pick(validationErrors, (inputValue, inputID) => (
Boolean(this.touchedInputs[inputID])
));
- this.setState({errors});
+
+ if (!_.isEqual(errors, this.state.errors)) {
+ this.setState({errors});
+ }
+
return errors;
}Screen.Recording.2022-11-22.at.20.54.25.mov |
|
The above fix will also fix any other form page using |
|
Thanks @mollfpr, I like the solution you outlined here #12766 (comment) better. Please commit that and I'll test again. |
|
I'll make that commit myself locally and start testing now so we can get this moving forward. |
|
Updated @neil-marcellini |
There was a problem hiding this comment.
We should update the max zip code length to 10 digits.
Line 107 in c948f3b
Maybe also add the hint here
Tests
After
edit.mov
draft.mov
fix-errors.mov
no-duplicate.mov
|
|
@mollfpr please update the zip code length and then I think it's good to go. |
|
@neil-marcellini Updated zip code |
…equestor-step-form
There was a problem hiding this comment.
I think this is good to go! @Santhosh-Sellavel tested on all platforms earlier and the changes since then have not been substantial. Great work @mollfpr.
Here's the offline test since I missed it earlier:
|
✋ 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 @neil-marcellini in version: 1.2.31-0 🚀
|
|
The production deploy comment failed for this PR, but this was deployed to production on v1.2.32-2 on Nov 28. |
| onChangeText={value => props.onFieldChange({zipCode: value})} | ||
| errorText={props.errors.zipCode ? props.translate('bankAccount.error.zipCode') : ''} | ||
| maxLength={CONST.BANK_ACCOUNT.MAX_LENGTH.ZIP_CODE} | ||
| hint={props.translate('common.zipCodeExample')} |
There was a problem hiding this comment.
Hi, we should have used zipFormatter here to have consistent hint messages.
Using it would have prevented #16912
There was a problem hiding this comment.
@rushatgabhane 😆 It would not have been possible to use that here, it was added only a couple of months ago.
Details
Refactor the personal information form (Requestor Step Form) to use
Form.js.Fixed Issues
$ #9581
$ #9581 (comment)
Tests
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)
Verify that no errors appear in the JS console
Offline tests
QA Steps
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)
Verify that no errors appear in the JS console
PR Author Checklist
I linked the correct issue in the
### Fixed Issuessection aboveI wrote clear testing steps that cover the changes made in this PR
TestssectionOffline stepssectionQA stepssectionI included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
toggleReportand notonIconClick)src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedIf a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarare working as expected)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:
/** 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)If a new CSS style is added I verified that:
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 author checklist, including those that don't apply to this PR.
PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionOffline stepssectionQA 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 usingAvatarhave been tested & I retested again)/** 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)Screenshots
Web
12766.Web.mov
Mobile Web - Chrome
12766.mWeb-Chrome.mov
Mobile Web - Safari
12766.mWeb-Safari.mov
Desktop
12766.Desktop.mov
iOS
12766.iOS.mov
Android
12766.Android.mov