update: payment method test mode label behavior #9647
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: +112 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
frosso
requested review from
a team and
danielmx-dev
and removed request for
a team
|
|
||
| <?php if ( WC_Payments::mode()->is_test() ) : ?> | ||
| if ( WC_Payments::mode()->is_test() && false !== $this->gateway->get_payment_method()->get_testing_instructions() ) : |
There was a problem hiding this comment.
I updated this so that the div.testmode-info element will be shown only if test instructions are present - otherwise it's displayed even on Klarna, Affirm, etc.
| $script = 'dist/checkout'; | ||
|
|
||
| WC_Payments::register_script_with_dependencies( 'wcpay-upe-checkout', $script, $script_dependencies ); |
There was a problem hiding this comment.
Inlined the $script, since it's not used anywhere else
| @@ -58,7 +58,7 @@ public function get_payment_method_script_handles() { | |||
| 'wc-blocks-checkout-style', | |||
| plugins_url( 'dist/blocks-checkout.css', WCPAY_PLUGIN_FILE ), | |||
| [], | |||
| '1.0', | |||
| WC_Payments::get_file_version( 'dist/checkout.css' ), | |||
There was a problem hiding this comment.
Getting the version from the build file, so that it changes at each release and we can effectively cache bust it - otherwise we incur the risk of providing an older build (because it could be cached by the customer's browser or the merchant's CDN)
| @@ -581,16 +581,15 @@ public function get_title() { | |||
| $title = parent::get_title(); | |||
|
|
|||
| if ( | |||
| Payment_Method::CARD === $this->stripe_id && | |||
There was a problem hiding this comment.
The "Test mode" badge is now displayed on all methods, when test mode is enabled. It'll be displayed by CSS only when the payment method is selected.
| } | ||
|
|
||
| .test-mode.badge { | ||
| display: inline-block; | ||
| display: none; |
There was a problem hiding this comment.
not displaying it by default - it'll be displayed only when the method is selected.
| } | ||
| } | ||
|
|
||
| li.wc_payment_method:has( .input-radio:not( :checked ) |
There was a problem hiding this comment.
Moved above - the different structure shouldn't be needed anymore.
| width: 100%; | ||
| &__pmme-container { | ||
| width: 100%; | ||
| pointer-events: none; |
There was a problem hiding this comment.
Added to ensure that clicking on the Payment Method Messaging Element expands the payment method
| title={ upeConfig.title } | ||
| countries={ upeConfig.countries } | ||
| iconLight={ upeConfig.icon } | ||
| iconDark={ upeConfig.darkIcon } |
There was a problem hiding this comment.
I just like to be explicit 🤷
| @@ -12,9 +12,50 @@ import './style.scss'; | |||
| import { useEffect, useState } from '@wordpress/element'; | |||
| import { getAppearance } from 'wcpay/checkout/upe-styles'; | |||
|
|
|||
| export default ( { api, upeConfig, upeName, upeAppearanceTheme } ) => { | |||
| const bnplMethods = [ 'affirm', 'afterpay_clearpay', 'klarna' ]; | |||
| const PaymentMethodMessageWrapper = ( { | |||
There was a problem hiding this comment.
Should this be on its own file?
There was a problem hiding this comment.
I personally don't think so, because it's not going to be used anywhere else - I just extracted it from the previous implementation because I thought it would make things slightly easier to scroll through, since there are a lot of if conditions to check for. 🤷
client/checkout/classic/style.scss
Outdated
| li.payment_method_woocommerce_payments { | ||
| display: grid; | ||
| grid-template-columns: 0fr 0fr 1fr; | ||
| grid-template-rows: max-content; | ||
|
|
There was a problem hiding this comment.
I originally used grid on the li element to prevent layout issues with themes that don't hide the input[type=radio] element and replace it with a custom ::before icon in the label.
If you switch to a theme like twenty-twentyfour you'll see that the layout is broken due to this.
This is how it looks like in develop.
There was a problem hiding this comment.
Thank you for catching that bug @danielmx-dev ! I think I got that covered, let me know what you think!
I ended up having to change the markup, and create a wrapper (with display: grid) for all the elements we want to display.
I found this approach to be working a little better across different themes, it was quite the headache 😅
Examples
| } | ||
|
|
||
| if ( targetLabel ) { |
There was a problem hiding this comment.
Changing this slightly - the container will always be present (generated from the backend), the JS will use it to mount the BNPL messaging.
|
|
||
| $bnpl_messaging_container = ''; | ||
| if ( $this->payment_method->is_bnpl() ) { | ||
| $bnpl_messaging_container = '<span id="stripe-pmme-container-' . $this->payment_method->get_id() . '" class="stripe-pmme-container"></span>'; | ||
| } | ||
|
|
||
| // the "plain" payment method label is displayed on some sections of the app | ||
| // - like "pay for order" when a payment method is pre-selected or a payment has previously failed. | ||
| $html = '<span class="woopayments-plain-payment-method-label">' . $title . '</span>'; | ||
| $html .= '<div class="woopayments-rich-payment-method-label">'; | ||
| $html .= '<div class="label-title-container">'; | ||
| $html .= '<span class="payment-method-title"> ' . $title . '</span>'; | ||
| $html .= $test_mode_badge; | ||
| $html .= '</div>'; | ||
| $html .= $this->get_icon(); | ||
| $html .= $bnpl_messaging_container; | ||
| $html .= '</div>'; | ||
|
|
||
| return $html; |
There was a problem hiding this comment.
Changing the previous approach a bit - I'm now rendering two elements: a "rich" and a "plain" element.
It turns out that the pay-for-order page will use this method to render the payment method (if a previous attempt failed).
So I need to render both labels and (via CSS) selectively hide/show the most appropriate one based on the context the label is displayed.
| @@ -12,9 +12,50 @@ import './style.scss'; | |||
| import { useEffect, useState } from '@wordpress/element'; | |||
| import { getAppearance } from 'wcpay/checkout/upe-styles'; | |||
|
|
|||
| export default ( { api, upeConfig, upeName, upeAppearanceTheme } ) => { | |||
| const bnplMethods = [ 'affirm', 'afterpay_clearpay', 'klarna' ]; | |||
| const PaymentMethodMessageWrapper = ( { | |||
There was a problem hiding this comment.
I personally don't think so, because it's not going to be used anywhere else - I just extracted it from the previous implementation because I thought it would make things slightly easier to scroll through, since there are a lot of if conditions to check for. 🤷
|
I forgot to mention that the radio button's circle is now displayed centered when the payment method is rendered in multiple lines, like for BNPLs: The behavior seems to be consistent with the blocks checkout: |
|
@frosso It looks like the hidden label is causing issues with the playwright tests. You may need to check the visibility or match using a different locator to fix the tests. |
|
I have tested different scenarios using both themes (Storefront and Twenty twenty-four) and they all look good! ✅ Classic Checkout I can approve this once the issue with the Playwright tests is resolved. |
|
@danielmx-dev thank you - that was a good call. |
| xPathPaymentMethodSelector | ||
| ); | ||
| await paymentMethodLabel.click(); | ||
| await selectOnCheckout( providerId, page ); |
There was a problem hiding this comment.
Thanks for refactoring this, I know this suite in particular is flaky so hopefully this improves the reliability.
|
For the record, are the labels still offset? |
|
@pierorocca the labels should be consistently spaced at checkout. Here's an example with WooCommerce Core, WooPayments, and WooCommerce Stripe payment methods displayed at checkout on two different themes: |
|
Cool ty! |
|
I've noticed on mobile the logo vertically centered alignment does have an impact on text wrapping. Mind adding a 375px viewport example? |
|
@pierorocca sure - these are a couple of examples on Storefront and TT4 with different payment methods selected (to showcase the label wrapping). I specifically hid some of the payment elements for the sake of ensuring things fit into the vertical space within the screenshot. 
EDIT: I re-uploaded the TT4 screenshots because I had some local changes affecting the result. |
|
Is there a bnpl example that shows the full offer detail copy with $ amounts? Ty. |
|
@pierorocca sure, here's an example with Storefront at 375px with the messaging and the "test mode" pill added/removed: |
Fixes #9625
Changes proposed in this Pull Request
Updated the "Test mode" badge to be displayed only when the payment method is selected.
I kept (but slightly modified) the grid layout.
I attempted a flexbox implementation, but ran out of options. The main challenge was showing the BNPL messaging on a new line, together with the card icon. I was semi-successful if I placed the card icon on
position: absoluteand thelabelonposition: relative, but it was becoming less maintainable.Changes are mainly UI-based, so unit tests are less effective.
Testing instructions
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