-
Notifications
You must be signed in to change notification settings - Fork 524
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
feat(wire): add wire bank modal to withdraw #6210
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.
Looks good to me. Were you able to test this in staging and confirm it properly implements the BE calls etc?
3b7a6b3
to
99a58de
Compare
@@ -191,7 +191,7 @@ class Deposit extends PureComponent<Props, State> { | |||
)} | |||
{this.props.step === BankDWStepType.WIRE_INSTRUCTIONS && ( | |||
<FlyoutChild> | |||
<WireInstructions {...this.props} handleClose={this.handleClose} /> | |||
<AddBankWireForm /> |
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.
Like this since we had a lot of such spread properties thing
</Entries> | ||
</div> | ||
|
||
{formValues.hasIntermediaryBank === 'YES' && ( |
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.
maybe move 'YES' to const
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 typed the hasIntermediaryBank
property so autocompletion/error handling alerts you of any error so I didn't deem it necessary
|
||
> div { | ||
flex: 7; | ||
} |
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.
why we need this it should be a wrapper itself
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.
In the latest version this is a component in the same file that is right below it so it's more explicit what it does, but if it's not clear I can create two new (Text)
wrappers
const formValues = useSelector((state) => | ||
formValueSelector('addWireBank')( | ||
state, | ||
'accountNumber', | ||
'bankName', | ||
'hasIntermediaryBank', | ||
'intermediaryAccountNumber', | ||
'intermediaryBankName', | ||
'intermediaryRoutingNumber', | ||
'routingNumber' | ||
) | ||
) as WireBankFormType |
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.
const formValues = useSelector((state) => | |
formValueSelector('addWireBank')( | |
state, | |
'accountNumber', | |
'bankName', | |
'hasIntermediaryBank', | |
'intermediaryAccountNumber', | |
'intermediaryBankName', | |
'intermediaryRoutingNumber', | |
'routingNumber' | |
) | |
) as WireBankFormType | |
const formValues = useSelector( | |
useMemo( | |
() => | |
formValueSelector('addWireBank')( | |
state, | |
'accountNumber', | |
'bankName', | |
'hasIntermediaryBank', | |
'intermediaryAccountNumber', | |
'intermediaryBankName', | |
'intermediaryRoutingNumber', | |
'routingNumber' | |
) as WireBankFormType, | |
[state] // Add dependencies as needed | |
) | |
); |
Consider using the useMemo hook to memoize the result of the formValueSelector to avoid unnecessary re-computations.
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.
Nice! It sent me digging in the docs and found out that redux
offers a shallowEqual
function that helps with this - see https://react-redux.js.org/api/hooks#equality-comparisons-and-updates
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.
Overall looks good to me, give it a try on staging if all works as expected, we can merge it 🙏
GROWUX-2401
This adds an "Add Wire Bank" flow to both withdraw/deposit from FIAT section.
Form won't allow moving forward if any field is empty and if Account Number is not 9 digits long where required.
Confirm screen only allows moving forward if checkbox is checked
User Bank Info
Intermediary Bank (if yes selected)
Information Screen (without Intermediary)
Information Screen (with Intermediary)
Success screen
Failure screen (default error)
Failure screen (already linked)