Use ECE buttons during style preview in settings when ECE is enabled #9028
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +517 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
… not served over HTTPS
… when https is not detected
reykjalin
requested review from
elizaan36,
nikkivias,
pierorocca,
a team and
cesarcosta99
and removed request for
a team
client/express-checkout/blocks/components/express-checkout-preview.js
Outdated
Show resolved
Hide resolved
client/settings/express-checkout-settings/payment-request-button-preview.js
Outdated
Show resolved
Hide resolved
client/express-checkout/blocks/components/express-checkout-preview.js
Outdated
Show resolved
Hide resolved
| if ( canRenderButtons ) { | ||
| return ( | ||
| <div | ||
| key={ `${ buttonType }-${ height }-${ theme }` } |
There was a problem hiding this comment.
I had to use the key prop here to force a rerender of the whole component when any of these props change because I found out that the <ExpressCheckoutElement> isn't updated when the options prop changes.
This causes an unfortunate "flash" where the button disappears entirely until it can be re-rendered with the new settings. But AFAICT there is no way around it.
Although I guess we could render several ECE's, each with a different configuration of the settings and the settings just dictate which ones have display: none set? 🤔
That probably wouldn't cause a flash like the current implementation, but it might make loading the page a little heavy? @pierorocca @elizaan36 @nikkivias what do we think here from a design perspective? (see video in description for a demo)
There was a problem hiding this comment.
Tested this a little more and was able to remove the height from the key. Also didn't need to include the border radius (now that #9010 is merged) so changing the height and border radius feels much better than the theme or CTA 🎉
There was a problem hiding this comment.
Here's a demonstration of the lag in re-rendering the buttons:
Screen.Recording.2024-07-08.at.6.38.46.PM.mov
There was a problem hiding this comment.
The flash is a little odd, especially since only the Apple Pay and Google Pay buttons are flashing.
That probably wouldn't cause a flash like the current implementation, but it might make loading the page a little heavy?
Could you share more on how loading the page would be heavy?
There was a problem hiding this comment.
Could you share more on how loading the page would be heavy?
Yes! The flashing is because the Stripe buttons are loaded in an iframe from Stripe's systems, and it takes a bit to initialize the buttons. The "solution" to the flashing I'm proposing would be to load all possible variations of the buttons, and hide them all via display: none;, except the one that matches the current configuration.
When the configuration is changed, instead of recreating the iframe with the buttons we would just hide the one that's currently visible and show the one that matches the selected configuration.
The page would be heavy to load because there would be 12 different iframes on the page, all showing different variations of the buttons (with only 1 of those visible at a time) instead of using just one iframe. Basically making the page slower to load, but it should be snappy once it's loaded.
I hope that's a good enough explanation, but please let me know if you have any further questions!
Important
Important disclaimer: I haven't tested the proposed solution here, but I think it should work.
There was a problem hiding this comment.
@elizaan36 @pierorocca @nikkivias just to make sure this isn't holding up merging this PR I've created #9091 to track this issue specifically and we can keep working on it from there, if we decide this is something we need to improve.
client/settings/express-checkout-settings/payment-request-button-preview.js
Outdated
Show resolved
Hide resolved
client/settings/express-checkout-settings/payment-request-button-preview.js
Outdated
Show resolved
Hide resolved
client/settings/express-checkout-settings/payment-request-button-preview.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I left some comments but this is looking nice and it's testing well 👍
- Make sure an info notice is displayed when no button can be rendered ✅
- Test button settings
⚠️ - I tested all combinations possible for the Apple Pay button ✅
- I tested all combinations possible for the Google Pay button and mostly worked fine. However, there might be a problem when the CTA is "Buy with", it looks like this is the only CTA that's not rendering accordingly. See images below.
| Only icon | Buy with | Donate with | Book with |
|---|---|---|---|
|
|
|
|
client/express-checkout/blocks/components/express-checkout-preview.js
Outdated
Show resolved
Hide resolved
client/express-checkout/blocks/components/express-checkout-preview.js
Outdated
Show resolved
Hide resolved
client/express-checkout/blocks/components/express-checkout-preview.js
Outdated
Show resolved
Hide resolved
client/settings/express-checkout-settings/payment-request-button-preview.js
Outdated
Show resolved
Hide resolved
For whatever reason the So I'm not sure why it's happening, but I suspect it has something to do with "buy" being the default use of the button so maybe they feel that it's redundant? EDIT: I think this has to do with the GPay button showing the last 4 digits of your credit card? You can see in the new recording of multiple buttons showing in the PR description that the narrower version of the button (no last 4 digits) has the correct CTAs. |
|
As for the design issues I'm going to open a separate issue for that. I think it'll be better to figure out what sort of layout we're going to implement and then update the preview to follow that, rather than doing something now with the preview that may not end up aligning with the design we arrive at. |
|
@pierorocca @bborman22 I've opened #9066 (comment) to track the remaining design issues we need to resolve. |
I'm not able to get the Venmo button to load on cart block and checkout block (though it loads on product and shortcodes). But looking into it, they do appear to register each button individually with Blocks. In my case with Venmo not loading that also creates a gap similar to what we were witnessing with ECE and exploring ways to address it.
|
There was a problem hiding this comment.
Thanks for addressing the requested changes 🎉 The code looks good, but a test case requires attention:
- Make sure an info notice is displayed when no button can be rendered
⚠️ - Check comments below.
- Test button settings ✅
- All combinations possible for the Apple Pay button
- All combinations possible for the Google Pay button
I think this has to do with the GPay button showing the last 4 digits of your credit card? You can see in the new recording of multiple buttons showing in the PR description that the narrower version of the button (no last 4 digits) has the correct CTAs.
I think you're right 👍 When I tested it on Safari (because now Google Pay is displayed there as well), where I'm not logged on my Google account, the "Buy with" CTA is being displayed.
client/settings/express-checkout-settings/payment-request-button-preview.js
Outdated
Show resolved
Hide resolved
client/settings/express-checkout-settings/payment-request-button-preview.js
Outdated
Show resolved
Hide resolved
It's a good callout @cesarcosta99. Apple announced at WWDC24 that they'll bring Apple Pay to all browsers on Mac OS this year. I don't know the timing but I'd expect Stripe to follow suit and support it. That would close a big chunk of non-supported browsers. Based on the Stripe supported browser list, the two most relevant scenarios where a button fails to display are:
For 1, display "To preview the Apple Pay button, view this page in the Safari browser." Is this an appropriate option? |
Thanks @bborman22. Do you think this is WooPayments caused or a general issue with the checkout block? |
@pierorocca @reykjalin 👋 The buttons should wrap and align left on the 2nd line. Not sure why they're aligning to the right in the screenshot here for blocks, this seems like a bug. |
I've seen this too in some of my testing. I don't know if this comes from WooPayments or not yet but it's worth looking into. Tends to happen in Safari and Firefox
I've just crated a PR to address the layout problem. Each button will have a minimum width of 240px, all buttons displayed equally widths, and a max of 2 columns. It'll look something like this Screen.Recording.2024-07-09.at.17.04.12.movFor me the buttons render ok with ECE when they are rendered individually. |
reykjalin
force-pushed
the
add/9007-use-ece-for-express-button-preview
branch
from
4863d33 to
3e8933b
Compare
@pierorocca We're removing the help text: #9028 (comment)
@elizaan36 It's a bug, but at the same time unavoidable in the current implementation. The way the new ECE components work is that multiple buttons are rendered in one container. The Checkout block has different breakpoints for wrapping content than Stripe's ECE container, so you end up with a weird layout. We're going to address this with #9045 and render multiple ECE containers so each ECE button will be rendered individually. |
That works when the insecure notice is presented. When it's a secure connection and there's no notice, as hightlighted here #9028 (comment) there are still 2 cases where the buttons shown don't match what the merchant is expecting to see. Are you planning to use the same inline notice with different copy? Or am I missing something? |
@pierorocca We always show a notice when a button isn't loaded, and I'd argue if the merchant is in Chrome they should only expect to see the Google Pay button. See the screenshots:
|
|
That works. There might be some merchant confusion in the short term based on support tickets I've seen in the past. As Apple and Stripe add non Safari support for Apple Pay Later this year that confusion will become a non-issue. The https version for Firefox is confusing to me but I'm fine with it to simplify the implementation. Firefox has a market share size too small to make it relevant. I appreciate the visuals, they helped a lot. |
There was a problem hiding this comment.
Thanks for all your hard work here! Code LGTM and all tests passed 🚀
- Make sure an info notice is displayed when no button can be rendered ✅
- Tested with:
- Firefox, Chrome and Safari.
- HTTP and HTTPS.
- WooPay and Apple Pay and Google Pay enabled and disabled.
- Tested with:
- Test button settings ✅
- All combinations possible for the Apple Pay button
- All combinations possible for the Google Pay button
…9028) Co-authored-by: Alfredo Sumaran <a.sumaran@gmail.com>
Fixes #9007
Changes proposed in this Pull Request
Questions for design
Should we have the preview display both buttons when available or try to always limit to just 1 button?We're going to display all available buttons. See Use ECE buttons during style preview in settings when ECE is enabled #9028 (comment).Screen.Recording.2024-07-05.at.2.31.56.AM.mov
Screen.Recording.2024-07-03.at.10.58.13.PM.mov
Info notice
Testing instructions
Setup
npm run wp option set _wcpay_feature_stripe_ece 1.curl https://api.stripe.com/v1/payment_method_domains \ # Don't forget the `:` at the end! -u "<secret key, sk_xxx or rk_xxx>:" \ -H "Stripe-Account: <merchant account ID>" \ # Example: my.domain.com -d domain_name="<your_url>"Make sure an info notice is displayed when no button can be rendered
woocommerce-payments/client/settings/express-checkout-settings/payment-request-button-preview.js
Line 225 in d8f1ee0
Test button settings
Important
You need to perform this test in both Chrome (or a Chromium based browser) logged into Google Pay and Safari with Apple Pay enabled.
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge