-
Notifications
You must be signed in to change notification settings - Fork 69
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
Set min width for ECE Google Pay button in blocks checkout #9124
Conversation
@pierorocca @nikkivias @alexflorisca Since you've all been taking a look at Express Checkout button rendering recently I wanted to run this approach by you all and get some opinions. The problem should be laid out well in the PR description, but in short, Google Pay through Stripe ECE requires a minimum width to render or else Stripe hides it automatically. In some scenarios (narrow checkout) the grid calculations would prevent Google Pay from rendering. Our proposed solution is to adjust this minimum column width to allow Google Pay to render when using ECE. @ricardo No need to review this PR yet, but wanted to confirm with you all that this matches what was discussed and that this is the right way to apply it. cc @rafaelzaleski FYI. |
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: +109 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
Looks good to me @bborman22. Awesome catch during the test exploration. Your solution looks aligned with the current design guidelines. @danielvann777 you provided a perspective on the existing design choice to left align the lone button. We could instead center align here as we finalize this migration effort. If we were to make a change, this would be great timing if the design team thinks this change is warranted. FYI @nikkivias FYI the WooPay font size is being increased in a different PR/Issue. Here it's smaller than desired. |
@bborman22 Yes, it matches, but for me the Google Pay button is hidden when the container width is below ece-button-in-grid.mov |
Hey @bborman22 thank for this work, but I've already got a PR up for addressing this (woocommerce/woocommerce#49304). In my testing, the button needed 240px of space so that's what I gave it. There seems to be a bit of variation on this reading the comments above, I say we go with the largest of them to be sure. |
Hey Alex does that hold up for translated buttons? |
Sorry for the delay here, @alexflorisca given ECE will be releasing with WooPayments 8.0 on July 31st should we include this fix as a temporary solution until the WC Core fix is in? This way the buttons don't get hidden during that gap? And I agree on going with the largest of our results which seems to be 240px as you said! |
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.
Great job! LGTM!
Sure thing |
Sorry for the lateness here @pierorocca. I would ask Nikki for the final say here since she probably knows all the permutations here but it does feel like a centered version would feel better in balancing the three buttons. |
@pierorocca for 3 buttons, would it be okay if the first button were to be full width, while the remaining 2 split the space at the bottom? I ask "would it be ok" because this layout gives preference to first button, and I'm not sure if they can currently assign the order of buttons. |
On WooPayments the most buttons that can dispaly currently is 3 so it works well as WooPay would be at the top. If we add other buttons in the future, there's a chance that Apple or Google or other brands could be placed more favorably than another competing brand. Technically, that violates their usage guidelines. For our own brand, I'm willing to go there. To put Amazon pay more prominently than Google or Google over Apple or any other combo, that's tricky especially if we have any sort of incentive agreements or partnerships in place. |
Again pointing out that this is pretty difficult to achieve as we don't know the number of payment extensions installed on a merchant's website, and the number of express payment methods that will be displayed. Our grid currently treats each of those equally and adds / removes grid items as payment methods are activated / deactivated. This layout may be possible with some client side logic to detect the WooPay button, place it first and make it take up two spaces in the grid instead of one. But it won't be possible to do this generically. |
Fixes #
Changes proposed in this Pull Request
This was discovered during our exploratory Express Checkout Element (ECE) testing. When testing with Storefront theme with the sidebar enabled, the checkout blocks page becomes quite narrow. This narrowness combined with WC Core's
grid-template-columns
on the.wc-block-components-express-payment__event-buttons
container lead to the spacing for the Google Pay button to be too small and automatically hidden by Stripe.To account for this we determined the minimum width the Google Pay button needs to render (208px) and replaced the calculated minimum width on the
grid-template-columns
with this fixed minimum width.This applies only to narrow checkout blocks pages (Storefront with sidebar enabled) and when more than 2 express checkout options are enabled.
Before
CSS:
grid-template-columns: repeat(auto-fit, minmax(calc(33% - 10px), 1fr));
When three or more items were present in the Express Checkout (i.e. WooPay, Google Pay, and PayPal) the 33% - 10px was not wide enough for Google Pay to render leading to this display:
When breaking below the media query breakpoint the CSS becomes
grid-template-columns: 1fr
and renders as a single column and all buttons:After
CSS:
grid-template-columns: repeat( auto-fit, minmax( 208px, 1fr ) ) !important;
The 208px is determined through experimentation and was found to be the minimum size that still renders the Google Pay button on ECE. This now leads to a single column list rendering like this:
When breaking below the media query breakpoint, there is a point where it goes into two columns before going back to a single column:
Here is what it looks like with the sidebar disabled which I think is a more common display:
With sidebar and just two buttons:
Testing instructions
npm run changelog
to add a changelog file, choosepatch
to 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