Form refactor bankaccountstep #10900
Conversation
melvin-bot
bot
requested review from
iwiznia and
mananjadhav
and removed request for
a team
|
@mananjadhav conflicts resolved and latest main merged, and I have removed 2 different keys( cc: @luacmartins |
|
I'll check this today again @jayeshmangwani . |
There was a problem hiding this comment.
LGTM overall, left a comment about the submit button that remains visible when Plaid is still loading or no account has been selected. Also, inside this function loading should be changed to isLoading so that it can be used from Form.
src/components/Form.js
Outdated
| @@ -40,6 +40,9 @@ const propTypes = { | |||
| // eslint-disable-next-line react/forbid-prop-types | |||
| draftValues: PropTypes.object, | |||
|
|
|||
| /** Should we show the form button */ | |||
| isSubmitButtonVisible: PropTypes.bool, | |||
There was a problem hiding this comment.
I think this needs to be moved below submitButtonText, as now it sits under the /* Onyx Props */ comment but it's not an Onyx prop.
There was a problem hiding this comment.
this change has been done
| @@ -83,9 +86,10 @@ class BankAccountStep extends React.Component { | |||
| return errors; | |||
| } | |||
|
|
|||
| validatePlaidAccount(values) { | |||
| validatePlaidAccount() { | |||
There was a problem hiding this comment.
I think we can skip validating Plaid account, as we don't allow the user to continue unless they choose an account. I don't see a way of triggering the validation error if it exists. @luacmartins what do you think?
There was a problem hiding this comment.
Yes I think we can skip this, removed validatePlaidAccount for now
There was a problem hiding this comment.
Makes sense since we can't display that error and the user can't continue without selecting an account.
| @@ -238,7 +227,6 @@ class BankAccountStep extends React.Component { | |||
| {subStep === CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID && ( | |||
| <Form | |||
| formID={ONYXKEYS.REIMBURSEMENT_ACCOUNT} | |||
| validate={this.validatePlaidAccount} | |||
There was a problem hiding this comment.
Removing this completely will cause an error, maybe we can have it return an empty object validate={() => ({})} to satisfy Form. I still think we don't need that validation method because it will only run when selectedPlaidBankAccount is already defined.
There was a problem hiding this comment.
added empty object to validate in PLAID Form
|
@jayeshmangwani we have conflicts again! |
|
@luacmartins conflicts resolved |
|
@youssef-lr Would you be able to get this? We should try to get this merged before we have to resolve conflicts again. |
|
We're not displaying any error from the server. This is because we started returning Screen.Recording.2022-09-29.at.22.16.51.mov |
|
@jayeshmangwani feel free to merge contents of Form and FormActions into this PR then we should be all set to merge! |
@youssef-lr latest code merged from main, and checked plaid and manual setup , its working fine, any changes I have to do for FormActions ? |
|
So the files I've linked are from this PR and not from main, it hasn't been merged yet. The changes should support a new There is also a separate issue to bring some more changes to Form but it's not gonna be merged yet. So I think you can copy contents of the files I linked and push the changes then we can merge! |
jayeshmangwani
dismissed stale reviews from iwiznia and luacmartins
via
bd2e691
@youssef-lr Done |
|
@youssef-lr fixed newline lint issue |
|
✋ 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 @youssef-lr in version: 1.2.11-0 🚀
|
|
@jayeshmangwani @youssef-lr @luacmartins Did you guys QA it internally? If not, can you help with QA steps. I am not able to follow what exactly we can validate here |
|
@mvtglobally I just QA'd this and it's a pass. |
|
🚀 Deployed to production by @AndrewGable in version: 1.2.11-4 🚀
|
…bankaccountstep Form refactor bankaccountstep
|
👋 Hi, we've identified this PR as 1/2 of the cause of #14972. This PR changed the naming convention from In this case, this could've been avoided by retaining the old naming convention, or by doing some more robust testing to ensure that both the draft and state values matched what we were expecting in a variety of scenarios. We've got a proposed regression test here that should cover the latter case 🙌 |
| @@ -75,6 +80,11 @@ class Form extends React.Component { | |||
| this.touchedInputs[inputID] = true; | |||
| } | |||
|
|
|||
| getErrorMessage() { | |||
| const latestErrorMessage = ErrorUtils.getLatestErrorMessage(this.props.formState); | |||
| return this.props.formState.error || (typeof latestErrorMessage === 'string' ? latestErrorMessage : ''); | |||
There was a problem hiding this comment.
Doing a refactor in #20228 , I got an eslint error related to this line :
ESLint: 'formState.error' is missing in props validation (react/prop-types)I guess this should be updated to this.props.formState.errors . But as I don't have much context, I would appreciate further clarification on this issue.
There was a problem hiding this comment.
@jayeshmangwani feel free to merge contents of Form and FormActions into this PR then we should be all set to merge!
I am not sure about this change, I copied these changes from this Form.js file and @youssef-lr may have the full context of these changes
There was a problem hiding this comment.
We're removing this.props.formState.error from that line in https://github.com/Expensify/App/pull/20091/files. See this comment.
Details
Fixed Issues
$ #9579
Tests
Verify that:
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** 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)PR Reviewer Checklist
The Contributor+ 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
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)
Verify that no errors appear in the JS console
Screenshots
Web
9579-web.mov
Mobile Web
9579-mWeb.mp4
Desktop
9579-desktop.mov
iOS
9579-ios.mov
Android
9579-Android.mp4