Show Express Checkout button previews in editor #10141
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: +741 B (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
danielmx-dev
requested review from
a team and
frosso
and removed request for
a team
There was a problem hiding this comment.
A few comments about memoization and dependencies I thought would be worth pointing out.
The fallback works well 👍 which makes me wonder - should we just use this fallback in preview mode, without worrying about the Stripe "preview" (which is not really a "preview")?
| }, FALLBACK_BUTTON_WAIT_TIME ); | ||
|
|
||
| return () => clearTimeout( handle ); | ||
| }, [ isPreview, onElementsReadyCalled ] ); |
There was a problem hiding this comment.
| }, [ isPreview, onElementsReadyCalled ] ); | |
| }, [ isPreview ] ); |
The value returned by useRef shouldn't be a hook dependency.
| if ( typeof buttonAttributes !== 'undefined' ) { | ||
| override.buttonHeight = Number( buttonAttributes.height ); | ||
| } | ||
| const checkoutElementOptions = useMemo( () => { |
There was a problem hiding this comment.
I'm not sure useMemo is too useful, in this scenario. It seems that the memoized value would be discarded at each re-render, regardless.
buttonOptions comes from the useExpressCheckout hook. And, in turn, it is computed by the getExpressCheckoutButtonStyleSettings() utility.
The getExpressCheckoutButtonStyleSettings utility returns a new object each time it is invoked.
So, buttonOptions will have a new in-memory reference each time the hook/utility is invoked, making useMemo also execute each time the ExpressCheckoutComponent component re-renders.
I noticed that buttonAttributes prop is also re-computed at each re-render: https://github.com/woocommerce/woocommerce/blob/3fced787bf20112dae420cb97b3da617a929b7de/plugins/woocommerce-blocks/assets/js/blocks/cart-checkout-shared/payment-methods/express-payment-methods.tsx#L33-L38
Is there a specific scenario you were thinking useMemo would have been useful in this component?
There was a problem hiding this comment.
This was leftover from when why was trying another approach. I'll remove the useMemo usage.
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { getExpressCheckoutButtonAppearance } from 'wcpay/express-checkout/utils'; |
There was a problem hiding this comment.
Since this file is within the tokenized-express-checkout, it's best to include the functions from that directory, rather than the express-checkout directory (which doesn't include "tokenized" cart changes).
Otherwise the bundles are going to get mixed, and there might be modifications in the "tokenized" functions that wouldn't be considered.
| import { getExpressCheckoutButtonAppearance } from 'wcpay/express-checkout/utils'; | |
| import { getExpressCheckoutButtonAppearance } from '../../utils'; |
| const appearance = useMemo( | ||
| () => getExpressCheckoutButtonAppearance( buttonAttributes ), | ||
| [ buttonAttributes ] | ||
| ); |
There was a problem hiding this comment.
Similar observation as the one I made in ExpressCheckoutComponent - is there a scenario where this useMemo would be useful?
There was a problem hiding this comment.
In this scenario I did it to avoid recalculating the border radius with every render, but the savings are probably negligible, I'll just remove it and see how it performs.
| () => getExpressCheckoutButtonAppearance( buttonAttributes ), | ||
| [ buttonAttributes ] | ||
| ); | ||
| const ref = useRef( null ); |
There was a problem hiding this comment.
Would it be possible to use a less generic ref name for this variable?
It looks like it's used for the GooglePlay's container - maybe googlePlayContainerRef or googlePlayWrapperRef?
| ref.current.appendChild( button ); | ||
| } )(); | ||
| } | ||
| }, [ ref, theme, expressPaymentMethod, borderRadius ] ); |
There was a problem hiding this comment.
| }, [ ref, theme, expressPaymentMethod, borderRadius ] ); | |
| }, [ theme, expressPaymentMethod, borderRadius ] ); |
Return values from useRefs shouldn't be used as part of hook dependencies.
| ref.current | ||
| ?.querySelector( 'button' ) | ||
| ?.style?.setProperty( 'border-radius', borderRadius ); | ||
| }, [ ref, borderRadius ] ); |
There was a problem hiding this comment.
| }, [ ref, borderRadius ] ); | |
| }, [ borderRadius ] ); |
| expressPaymentMethod === 'googlePay' && | ||
| ! renderGooglePayButtonPromise.current | ||
| ) { | ||
| renderGooglePayButtonPromise.current = ( async () => { |
There was a problem hiding this comment.
I'm curious about the assignment of this function to renderGooglePayButtonPromise.current - could we have gotten away with just assigning a flag to indicate if the loading had been called?
i.e.: something like this:
diff --git a/client/tokenized-express-checkout/blocks/components/express-checkout-button-preview.js b/client/tokenized-express-checkout/blocks/components/express-checkout-button-preview.js
index b612e0202..1fce7c0d4 100644
--- a/client/tokenized-express-checkout/blocks/components/express-checkout-button-preview.js
+++ b/client/tokenized-express-checkout/blocks/components/express-checkout-button-preview.js
@@ -18,7 +18,7 @@ const ExpressCheckoutButtonPreview = ( {
[ buttonAttributes ]
);
const ref = useRef( null );
- const renderGooglePayButtonPromise = useRef( null );
+ const hasStartedLoadingGooglePlayButton = useRef( false );
const theme = options.buttonTheme[ expressPaymentMethod ];
const borderRadius = appearance.variables.borderRadius;
@@ -27,9 +27,10 @@ const ExpressCheckoutButtonPreview = ( {
if (
ref.current &&
expressPaymentMethod === 'googlePay' &&
- ! renderGooglePayButtonPromise.current
+ ! hasStartedLoadingGooglePlayButton.current
) {
- renderGooglePayButtonPromise.current = ( async () => {
+ hasStartedLoadingGooglePlayButton.current = true;
+ ( async () => {
const targetDocument = ref.current.ownerDocument;
const targetWindow = targetDocument.defaultView;
if ( ! targetWindow.google?.payments?.api?.PaymentsClient ) {
My initial reasoning was to always try using Stripe first to provide a Preview as close as possible to the live experience; however, at least from my testing, the appearance in both are really close, so it's probably fine to use the new component in all previews. |
There was a problem hiding this comment.
Excellent work! Thank you for entertaining my suggestions.
I added a very optional suggestion to simplify the body of the ExpressCheckoutButtonPreview component.
Now that we have ExpressCheckoutButtonPreview, I am even questioning whether ExpressCheckoutButtonPreview should be rendered within ExpressCheckoutComponent. In the edit property of tokenizedExpressCheckoutElementApplePay/tokenizedExpressCheckoutElementGooglePay we could just render ExpressCheckoutButtonPreview and get rid of the whole isPreview prop 🤷 but I'm guessing that would be too many changes in this PR.
Regardless, this already fixes the main issue 👍
client/tokenized-express-checkout/blocks/components/express-checkout-component.js
Outdated
Show resolved
Hide resolved
client/tokenized-express-checkout/blocks/components/express-checkout-button-preview.js
Outdated
Show resolved
Hide resolved
|
@frosso Based in this comment, I added one more commit to avoid using the Preview component if a new express payment method gets added in the future and doesn't have an implementation yet. f26c0ad |
|
Sounds good, thank you @danielmx-dev . Looks like a minor change 👍 it would have been fine to also add that in the future, if we needed to implement those methods :D |
|
@frosso It looks like we'll finally move forward with this solution. I have re-requested your review since I copied the solution to non-tokenized ECE as well. |
danielmx-dev
deleted the
fix/show-preview-for-express-checkout-buttons-in-editor
branch
Fixes #10043
Changes proposed in this Pull Request
When visiting the Page: Cart template, the button for express payment is not visible in the Cart block. When debugging this further, I noticed that when using the template editor, the contents were rendered inside an iframe, which Gutenberg does by design: WordPress/gutenberg#25775
When the Stripe iframe is located inside the FSE iframe, no callbacks are executed and the payment buttons are never loaded (even if the conditions to render the buttons are met). This is not a problem with the page editor since, in my testing, that editor does not use an iframe to render the contents of the page, so the buttons load as expected.
In this PR I'm proposing to establish a timeout, if the elementsReady callback is never called, we render fallback buttons using the APIs provided by Google Pay (the Pay javascript library and https://developers.google.com/pay/api/web/guides/brand-guidelines ) and ApplePay (proprietary CSS rules only available in Safari https://developer.apple.com/documentation/apple_pay_on_the_web/displaying_apple_pay_buttons_using_css).
The downside of this approach is that, if more express payment methods are added, we'd need to implement a way to render the buttons ourselves. An alternative would be to just show placeholder buttons and don't worry too much about the styling, but please let me know what do you think.
Testing instructions
Previous Result:
In the page editor, the supported Express Checkout buttons are rendered
In the template editor no buttons are rendered, and there's only a blank space:
Current Result (using this branch):
The Page editor remains the same, and buttons are rendered:
The Template editor now renders the express payment buttons:
Regression Testing:
Express Payment method should still work as expected for making purchases in the Product, Cart, and Checkout pages.
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