Implement Stripe CC and Stripe ApplePay payment methods #1983
Conversation
nerrad
added
focus: components
nerrad
force-pushed
the
add/implement-stripe-apple-pay
branch
from
d26889c to
728cdf3
Compare
nerrad
force-pushed
the
add/stripe-cc-payment-method
branch
from
9e90ae5 to
bd800ce
Compare
nerrad
force-pushed
the
add/implement-stripe-apple-pay
branch
from
728cdf3 to
a27467a
Compare
nerrad
force-pushed
the
add/stripe-cc-payment-method
branch
from
bd800ce to
e0ca954
Compare
assets/js/payment-methods-demo/express-payment/apple-pay/apple-pay-express.js
Outdated
Show resolved
Hide resolved
assets/js/payment-methods-demo/payment-methods/stripe/payment-method.js
Outdated
Show resolved
Hide resolved
assets/js/payment-methods-demo/payment-methods/stripe/payment-method.js
Outdated
Show resolved
Hide resolved
assets/js/payment-methods-demo/payment-methods/stripe/payment-method.js
Outdated
Show resolved
Hide resolved
nerrad
force-pushed
the
add/implement-stripe-apple-pay
branch
from
a27467a to
a44f244
Compare
nerrad
force-pushed
the
add/stripe-cc-payment-method
branch
from
e0ca954 to
4eeea99
Compare
nerrad
force-pushed
the
add/implement-stripe-apple-pay
branch
from
a44f244 to
5a43c8f
Compare
nerrad
force-pushed
the
add/stripe-cc-payment-method
branch
from
4eeea99 to
b37a7a9
Compare
nerrad
force-pushed
the
add/implement-stripe-apple-pay
branch
from
5a43c8f to
ffcabaf
Compare
nerrad
force-pushed
the
add/stripe-cc-payment-method
branch
from
9a055a5 to
61cc733
Compare
|
Size Change: -105 kB (4%) Total Size: 2.15 MB
ℹ️ View Unchanged
|
nerrad
force-pushed
the
add/implement-stripe-apple-pay
branch
from
ffcabaf to
a1ca8b3
Compare
nerrad
force-pushed
the
add/stripe-cc-payment-method
branch
from
61cc733 to
5fbb2b2
Compare
nerrad
force-pushed
the
add/implement-stripe-apple-pay
branch
from
a1ca8b3 to
5206f57
Compare
nerrad
force-pushed
the
add/stripe-cc-payment-method
branch
from
5fbb2b2 to
bc2fb74
Compare
nerrad
force-pushed
the
add/implement-stripe-apple-pay
branch
from
5206f57 to
3abbda1
Compare
nerrad
force-pushed
the
add/stripe-cc-payment-method
branch
2 times, most recently
from
278d58d to
5cc5b43
Compare
If payment methods use these classes for their fields then the styles will get applied. It _could_ allow for consistent styling, we may have to provide design documentation for this? These are styles in cases where payment methods have to use elements provided by the gateway (eg. Stripe elements). In future iterations we could look at providing components to payment methods to use (if they aren’t restricted by the gateway).
…owser supports it
nerrad
force-pushed
the
add/stripe-cc-payment-method
branch
from
44ca2fe to
a2e855e
Compare
|
@haszari The issues you were experiencing were due to the production build being broken (I'm assuming that's what you were testing with). The reason why I didn't see those issues is because I build my plugin zip based on the dev build (so I could see error stacks). It turns out that production was broken in master (not introduced by this branch) and thanks to help from @senadir , fixed in #2042. After #2042 was merged into master, I rebased this branch on top of it, so this should be good to go for testing again. |
|
@senadir regarding your testing (thanks for the notes!). I'm noting what I experienced when trying to reproduce. I'm thinking these can be handled in followups (those more skilled with css can tackle them?)
It wasn't clear what browser this was happening for you, so I tested Chrome, Safari and Firefox. The only browser I could reproduce this in was Safari. Not sure yet how we'll fix this for Safari.
The only browser I could reproduce this in was Firefox and even then, I can't reproduce reliably, sometimes it happens, other times it doesn't (no matter how I try to select and delete).
Logic handling when the button clicked is not expected to be functional in this pull, I'd planned on doing that as a followup. |
This allows us to more accurately demonstrate how payment extensions would hook in to the blocks.
| @@ -36,6 +36,7 @@ | |||
| width: 50%; | |||
| > img { | |||
| width: 100%; | |||
| height: 48px; | |||
There was a problem hiding this comment.
Should this be max-height? Are we setting an expected/standard dimensions for gateways to use?
There was a problem hiding this comment.
I think we want to ensure that there's some consistency in styling here. Otherwise multiple express payment methods could risk being varying heights?
| } | ||
| & + .wc-block-form-input-validation-error { | ||
| position: static; | ||
| margin-top: -$gap-large; |
There was a problem hiding this comment.
Is this working? I've had issues with this before (values became positive) and found a SO thread saying interpolation should be used. -#{$gap-large} I didn't test, just stood out.
There was a problem hiding this comment.
Working for me, maybe the webpack plugin handles it better than it used to?
| @@ -21,6 +21,7 @@ const Tabs = ( { | |||
| instanceId, | |||
| ariaLabel = __( 'Tabbed Content', 'woo-gutenberg-products-block' ), | |||
| children, | |||
| id, | |||
There was a problem hiding this comment.
Should we generate a default fallback for ID like on selectedId.
There was a problem hiding this comment.
This is mostly just an optional id for the entire tabs container. There's an issue (#1474) for reviewing/improving/replacing this tabs component I added, when we get to that at some point, we can use the opportunity to review whether this id is even needed (or improve).
| } from '../../stripe-utils'; | ||
|
|
||
| /** | ||
| * External dependencies |
There was a problem hiding this comment.
I've noticed in recent files you've worked on you've started defining internal before external - is there a reason for this?
There was a problem hiding this comment.
Ha! I always get mixed up and go with whatever I remember doing last. It didn't even occur to me I was doing it wrong. If the convention is external first (which makes sense) I should definitely be consistent with that. I created an issue (#2052)
| /** | ||
| * @type {[ StripePaymentRequest|null, function( StripePaymentRequest ):StripePaymentRequest|null]} | ||
| */ | ||
| // @ts-ignore |
There was a problem hiding this comment.
Yes. I have a personal todo (not an issue) to review all @ts-ignore usages and see about fixing. I just don't want to waste time on fixing things that is because I can't figure out how to properly document a particular type.
| { __( 'Credit/Debit Card', 'woo-gutenberg-products-block' ) } | ||
| </strong> | ||
| ), | ||
| activeContent: <StripeCreditCard />, |
There was a problem hiding this comment.
I think content would be fine for naming - I think you used active because it's a tab, but I don't think thats important when it comes to naming things in the common API.
There was a problem hiding this comment.
I didn't just use activeContent because it's in a tab. I used it because it's the component to render when the payment method is selected (and thus made active). However, I do realize that we could probably just get away with content here since there is no other "content" type (other than label which is already distinctive). I'll change.
| ), | ||
| activeContent: <StripeCreditCard />, | ||
| edit: <EditPlaceHolder />, | ||
| canMakePayment: stripePromise, |
There was a problem hiding this comment.
The naming of this one is slightly odd. I assume this is basically whether or not the method will be enabled or visible? Remember some gateways (offline, invoice etc) don't technically take payment.
There was a problem hiding this comment.
It's for a consistent api, because some payment methods can only make a payment given certain conditions on the environment (browser in use, configured properly, third party library loaded etc). This allows for a consistent way for gateways to indicate they can make payment. For offline, or invoice payment methods they could just do Promise.resolve( true ) as the value for this property.
| try { | ||
| stripe = loadStripe( getApiKey() ); | ||
| } catch ( error ) { | ||
| // eslint-disable-next-line no-console |
There was a problem hiding this comment.
Will be be a customer facing error or a silent one?
There was a problem hiding this comment.
If stripe fails to load here, then the payment method itself will not load or display anything. The reason why I commented this out is because it does a console error in the admin.... I need to revisit though, I'm going to add a @todo for a followup.
| * | ||
| * @return {StripeServerData} | ||
| */ | ||
| const getStripeServerData = () => { |
There was a problem hiding this comment.
So if I understand correctly, gateways might need to pass data to themselves via settings, but we need to ensure block checkout is loaded first?
How about a common API on the PHP side for gateways to insert variables, and then have either the checkout or payment provider load them in somehow and pass through to payment method on initialisation.
There was a problem hiding this comment.
I like that idea. Adding as a follow-up.
| @@ -212,17 +213,17 @@ | |||
| * @property {string} redirectUrl This is the url that | |||
| * checkout will redirect | |||
| * to when it's ready. | |||
| * @property {function()} onCheckoutCompleteSuccess Used to register a | |||
| * @property {function(function())} onCheckoutCompleteSuccess Used to register a | |||
There was a problem hiding this comment.
It means that the "type" for this value is a function that receives a function as it's argument.
| margin: 0 $gap; | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| max-width: calc(100% - #{ 2 * $gap }); |
There was a problem hiding this comment.
There seems to be a double space before the 2, I think starting/ending spacing in curly braces should be removed?
| max-width: calc(100% - #{ 2 * $gap }); | |
| max-width: calc(100% - #{2 * $gap}); |
I don't know why stylelint didn't catch those.
There was a problem hiding this comment.
I don't know why stylelint didn't catch those.
Yup no idea.
Fixes: #1471
This pull implements the Stripe ApplePay and Stripe CC payment methods in the Checkout block and gets us one step closer to having the blocks fully functional. There are still some todos after merging this pull but I'm submitting it for review now because:
Incomplete tasks:
This pull has some todos that are not complete. Once this pull is merged, I will go through this list (and inline todos) and determine what needs documented as issues.
SavedPaymentOptionscomponent so it doesn't pull actual saved payments (if they exist) for the logged in user when in the editor. Instead, it should use preview data and also default to create a new payment option (for visibility). (Add treatment for SavedPaymentMethodOptions in the editor context #2061)abstract-wc-stripe-payment-gateway::get_icon()for how the payment method extension currently determines what icons to show. (Stripe: Pull cc images from gateway. #2064)StripePromisefor the stripe payment methodcanMakePaymentlogic. #2065)Complete tasks
stripe.jsis wired up and hooked in to the current event emitters.editnode so payment methods can indicate what gets shown when in the editor view.Stripephp class server side that is used for the Stripe payment method integration server side.Screenshots
Here's some screenshots/gifs to give an overview of completed things (and to verify I tested what I could 😄 )
When there are no express payment methods that can pay (in this case Apple Pay is configured but I'm viewing checkout in the frontend in Chrome)
Field behaviour for Stripe Credit Card
Example of inline validation errors
Apple Pay in Safari on desktop
Apple Pay, Safari desktop, browser payment interface opened.
Apple Pay, Safari - iPhone X
ApplePay, Safari - iPhone X - browser payment interface opened.
Testing
The following are the important things you are testing:
To setup for testing
package-pluginnpm script included in our build tools.