-
Notifications
You must be signed in to change notification settings - Fork 57
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
New checkout components for multiple payments #388
Conversation
🚀 Preview deployed Built with commit ec655f1 https://deploy-preview-388--stoic-hodgkin-c0179e.netlify.com |
Only the order of the props changed
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 everything looks / works great. I few very minor comments here, none of which are blocking to getting this merged.
{ | ||
displayName: "Credit Card", | ||
InputComponent: StripePaymentInput, | ||
name: "stripe_card", |
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 do we use the _
style here as opposed to camel-case? Is it a stripe thing?
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'm not sure why we did but I think it goes back a long way. This has to match the method name that's defined by the plugin on the server, so not much we can do about it now.
this.validateShippingAddress = this.validateShippingAddress.bind(this); | ||
this.setShippingAddress = this.setShippingAddress.bind(this); | ||
this.setFulfillmentOption = this.setFulfillmentOption.bind(this); | ||
this.setPaymentMethod = this.setPaymentMethod.bind(this); |
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.
Should we update these functions in the CheckoutActions.js
file to not need to use bind()
anymore?
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 actually tried that but our normal method of defining them does not work in Styleguidist examples. There must be a Babel plugin that doesn't get applied on these code blocks. It's probably something we could remedy, but I didn't want to deal with it right now.
package/src/components/ExampleIOUPaymentForm/v1/ExampleIOUPaymentForm.js
Outdated
Show resolved
Hide resolved
package/src/components/PaymentsCheckoutAction/v1/PaymentsCheckoutAction.js
Outdated
Show resolved
Hide resolved
package/src/components/PaymentsCheckoutAction/v1/PaymentsCheckoutAction.js
Outdated
Show resolved
Hide resolved
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 with changes 👍
🎉 This PR is included in version 0.61.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Resolves #218
Resolves #219
Resolves #352
Impact: minor
Type: feature
Components
There are four new components:
AddressChoice
ExampleIOUPaymentForm
StripePaymentInput
PaymentsCheckoutAction
The code in these components, except for
ExampleIOUPaymentForm
, was split out and improved fromStripePaymentCheckoutAction
. The newPaymentsCheckoutAction
handles multiple payments and collection of the billing address.StripePaymentInput
is just the Stripe form andExampleIOUPaymentForm
is a similar form for theiou_example
payment method. These are essentially "plugin components" forPaymentsCheckoutAction
, and anyone developing a new payment method can easily make their own "plugin component" with the necessary fields to collect the input needed for that payment method.ExampleIOUPaymentForm
is not intended for production use.Other Changes
accounting-js
to format the payment amounts. These are displayed before they are ever sent to the server, so we can't rely on the server to provide a displayAmount.StripePaymentCheckoutAction
componentBreaking changes
None
Testing
Try all of the new components and check for errors or unexpected behavior. You can test all of them put together on the revised
CheckoutActions
full example.