Add support for button styling for ECE buttons #8988
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
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: +471 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
6 tasks
reykjalin
requested review from
a team and
rafaelzaleski
and removed request for
a team
reykjalin
added
pr: needs design review
needs design
|
@nikkivias @elizaan36 the screenshots of the different options are in #8951 (comment) |
We don't have direct control over font sizes, but we can set the base font size for Stripe's elements, so we can probably figure out a way to get the exact font size we want. I'm curious if this should be a separate issue though? Are the font size not different in the PRB implementation? 🤔 If they're different there I'd say this isn't a regression and shouldn't be fixed in this PR. |
rafaelzaleski
requested changes
Jun 24, 2024
I think it fits the scope of #8136. |
Ah yes, I agree!
Yes, that sounds like a nice solution. It'd ensure it scales with the size of the button effortlessly too 👍 |
rafaelzaleski
approved these changes
Jun 25, 2024
There was a problem hiding this comment.
The code looks good, and the settings are correctly applied to the ECE buttons. Nice work!
There are some design inconsistencies, but they are out of scope for this review and have already been addressed in issue #8951 with the design team, resulting in new issues being created.
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 #8951
Changes proposed in this Pull Request
No automated tests because it's hard to make assertions on iframes and their style.
Testing instructions
Important
There's an issue with Storefront that causes the button width to be incorrect. We're still discussing how to address that. In the meantime you can test with Twenty-Twenty Four. See #8951 (comment) for details.
Important
HTTPS is a hard requirement for testing the buttons. They will not be rendered unless you have a valid HTTPS URL to test with. Jurassic tube is recommended for permanent custom URLs.
Setup
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>"_wcpay_wcpay_feature_stripe_eceto1:wp option set _wcpay_feature_stripe_ece 1.Test button sizes
Note
Due to limitations in Stripe's API the maximum height of the buttons is
55pxinstead of the56pxsuggested by theLargeoption in the settings. We're discussing this in #8951 (comment).Test button theme (i.e. color)
Important
Stripe only supports an
Outlineversion of the Google Pay button so both theLightandOutlinethemes will have an outline.Note
You can keep the size the same while doing this test.
Test combinations
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