Update express checkout max button height to 55px #9117
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bborman22
requested review from
a team and
reykjalin
and removed request for
a team
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: +1 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
reykjalin
requested changes
Jul 18, 2024
There was a problem hiding this comment.
The buttons render at the correct height according to the devtools (see issue below) on the customer-facing pages, but the WooPay button is still 56px in the settings preview for me 🤔
I also see the ECE preview has it's min-height set to 56px:
I was able to fix the ECE preview min-height by changing the buttonSizeToPxMap:
I was able to fix the WooPay button by modifying the stylesheet for the button:
For some reason the WooPay button at 55px is smaller than the Google Pay ECE button at 55px???
Both are 55px according to the dev tools. Maybe the ECE buttons don't take the border into account so a 1px border means the end result is a height of 57px? That seems to be right:
Setting the max-height of the container to 55px results in an obvious cut-off:
It's only just barely noticable when they're side-by-side so I think we can get away with fixing this in a separate PR once all the correct changes have been made here. Or maybe this warrants a bug report to Stripe?
|
Great catch! Sorry I missed those preview changes, but was a quick fix like you suggested.
I would lean that way of a separate PR, but only because it seems like this only applies to Light / Outline mode. I was wondering if you could confirm that as well? And if we don't involve changes to Stripe, our fix would be the set WooPay button to 57px essentially? |
reykjalin
approved these changes
Jul 19, 2024
There was a problem hiding this comment.
Looks good! 🚀
I would lean that way of a separate PR, but only because it seems like this only applies to Light / Outline mode. I was wondering if you could confirm that as well?
Ah yes, nice catch! Yes, this seems to be limited to Light/Outline mode. I wonder if it's related to the approach Stripe needed to take for margins 🤔 #9005 (comment)
And if we don't involve changes to Stripe, our fix would be the set WooPay button to 57px essentially?
I think so? That, at least, seems to be the only way to make them match for the light and outline versions 🤔
Having to make that distinction is what held me back from addressing here. I created this issue to track it going forward. |
lovo-h
pushed a commit
that referenced
this pull request
Aug 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #9006
Changes proposed in this Pull Request
This is a small update to set the maximum button height to 55px for WooPay, Google Pay, and Apple Pay. This was the result of this discussion in which it was observed the Stripe Express Checkout Element (ECE) has a max height of 55px. In the linked issue it was determined we should match Stripe's setting here of 55px. This PR makes the adjustment so that all buttons under our control will share this max height for consistency.
Testing instructions
npm run wp option set _wcpay_feature_stripe_ece 1Large (55 px)Large (55 px). Inspect all buttons displayed and confirm they are rendering at 55px height.wcpayConfig.woopayButton.heightandwcpayExpressCheckoutParams.button.heightare both55. (Depending on some optionswcpayConfigmay need to bewcpay_upe_config).npm run wp option set _wcpay_feature_stripe_ece 0wcpayPaymentRequestParams.button.heightin this case.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