-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Form refactor bankaccountstep #10900
Form refactor bankaccountstep #10900
Conversation
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.
2 questions raised, @jayeshmangwani
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think we can skip this, removed validatePlaidAccount for now
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
bd2e691
@youssef-lr Done |
@youssef-lr fixed newline lint issue |
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.
Thanks for the changes!
✋ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're removing this.props.formState.error
from that line in https://github.com/Expensify/App/pull/20091/files. See this comment.
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.
That’s perfect.
Details
Fixed Issues
$ #9579
Tests
Verify that:
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is 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 Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is 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