-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add Payment Request button support for Woo blocks #1529
Add Payment Request button support for Woo blocks #1529
Conversation
9aec227
to
c8a8123
Compare
c8a8123
to
a079411
Compare
4104b3b
to
6d7755f
Compare
@asumaran Thanks for testing it. I was concerned the changes I made to the payment request button class could break your PR. |
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.
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.
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 is all coming along nicely! I think there's just some minor comments on the code, and the issues I pointed out ^ above.
I'm going to do some more testing, especially to make sure the shortcode flow is still working as expected following the changes to the API, but I think this is all generally looking pretty good
I really like how you've been able to adapt the Stripe implementation to fit better into the WCPay codebase. Excellent work! 👏
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 is sort-of unrelated to this PR, but I'm reporting it here anyway; @ricardo please let me know if I should move this to a different issue/PR
When I tried to pay through the credit card form with a card that will be declined (4000 0000 0000 0002
) I don't get the expected Card declined
error, and instead get this generic message at the top of the checkout page:
Error Message | Credit Card Form |
---|---|
The PRB shows the expected error message in the correct place.
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.
It seems the aforementioned issue with failing 3DS verification through a PRB might be an issue that existed before this PR, since the product page shows the exact same result:
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.
Related to the light-outline
button not showing up correctly it works on the Product page, so that's definitely new with this PR.
Please move to a new issue 👍 I have seen this error before, but didn't connect the dots. Well observed though. It's probably because the block credit card method is still using the generic Gutenberg checkout endpoint to create the order, and that's not throwing any specific response from Stripe. |
Great catch! I changed the behavior a little bit, after looking at how Stripe renders the actual Google Pay button. The I had to add a small vertical margin so that the Fixed in ffb0de4
The issue was actually generated from this PR 😬 Fixed it in eb615a7. |
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.
Just 2 minor comments on the updated PRB styling, but otherwise this looks good!
All the issues I encountered during my previous review have been fixed! Awesome work! 👏
Let's get that CSS fixed (possibly with the help of @LevinMedia) and then get this shipped 🎉
&:active { | ||
background-color: #fff; | ||
} | ||
&:hover { | ||
background-color: #f8f8f8; | ||
} | ||
&:focus { | ||
box-shadow: #e8e8e8 0 1px 1px 0, #e8e8e8 0 1px 3px; | ||
outline: 0; | ||
} |
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 need to include the box-shadow
here to indicate whether the element is focused when (for example) navigating via the tab key.
Personally, I think we should allow the purple outline set by WC Core, since that's really universal throughout WC pages; I don't see why we would remove that. Maybe @LevinMedia has some thoughts on that, design-wise?
The box-shadow
on its own is very difficult to notice.
We should also make sure this is the same across both Stripe and WCPay, so what we decide to do here, I will also do in Stripe. For now I just added the box-shadow
back in Stripe.
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.
Good point about the accessibility issue.
I removed the box-shadow
because the Google Pay button that Stripe renders (when you have the wallet available) doesn't have a box-shadow
.
Apparently, it uses the same styling as :hover
, so perhaps the right thing to do is to use:
&:hover, &:focus {
background-color: #f8f8f8;
}
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.
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.
The Stripe plugin version of this is not the best version of this either IMO, so I'd really like to get @LevinMedia's 👀 on this 🙂
I'll ping him in Slack 👍
EDIT: Nevermind, he's AFK until tomorrow so I'm not going to ping him now. Let's not leave this as a blocker, but if we haven't merged before he's back (tomorrow) I think it's worth it to have him take a look at this
@@ -46,13 +60,13 @@ | |||
background-color: #3c4043; | |||
} | |||
&:focus { | |||
box-shadow: #5f6368 0 1px 1px 0, #5f6368 0 1px 3px; | |||
outline: 0; | |||
} |
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.
Same as above ^, we need to include the box-shadow
at the very least, and maybe allow the outline too.
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.
3c2caa9
to
5632861
Compare
Co-authored-by: Kristófer Reykjalín <[email protected]>
bef480e
to
5b2980c
Compare
@reykjalin @ricardo - let's just use the styling that stripe provides for now - I'm hesitant to use a specific color outline for focus since it may not always work with a theme's styles 👍 |
}; | ||
|
||
export const shippingOptionChangeHandler = async ( api, event ) => { | ||
const response = await api.paymentRequestUpdateShippingDetails( event ); |
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.
@ricardo it should've been event.shippingOption
. I'm submitting a PR fixing it.
Fixes #1416
Fixes #1544
Fixes #1677
Changes proposed in this Pull Request
clearCart
, unnecessary i18n and nonce variables) and make shortcode integration more similar to the classic checkout by using theWCPayAPI
.There are a few improvements left to do:
client/blocks/
. - This requires changing the current credit card checkout as well.Testing instructions
npm run build:client
.