-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add support for buttonAttributes and set default height to 48px #175
Conversation
@@ -171,6 +179,7 @@ const ButtonComponent = () => { | |||
)} | |||
{!isGooglePayDisabled && ( | |||
<div | |||
style={{ height: `${buttonHeight}px` }} |
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.
Curious why the Google Pay button needs px
added but the Apple Pay one doesn't?
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.
Using a number instead of a string implies the px
unit. As for which one to use, I guess it's a matter of preference, maybe the latter is preferable since it's very clear that we are using pixels 🤷🏼 .
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.
That's actually an oversight on my part, good spot!
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.
Should be fixed now, both should use px values
QA Update ✅I have checked this issue in the I tested the following on this branch:
Testing Environment
Steps to Test- As mentioned in the PR description.
|
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.
Thanks for working on this @alexflorisca, it tests great.
@@ -171,6 +179,7 @@ const ButtonComponent = () => { | |||
)} | |||
{!isGooglePayDisabled && ( | |||
<div | |||
style={{ height: `${buttonHeight}px` }} |
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.
Using a number instead of a string implies the px
unit. As for which one to use, I guess it's a matter of preference, maybe the latter is preferable since it's very clear that we are using pixels 🤷🏼 .
Thank you all for the reviews and QA. I've made a small change as pointed out to make sure we pass |
This will go through a final round of regression testing with all other PRs that are ready to be released. Once that passes, we will get this merged in and a release prepped |
Regression / Smoke Test Report ✅Tested with Archive File created via "php woorelease.phar build repo_URL" (Composer version 2.5.5, npm version 8.19.4, node version 16.20.0) Status- Working expected with Plugin Archive/Zip file same as fix specific branch. Testing Environment
Next Step- Ready to Merge 🚀 |
All Submissions:
Changes proposed in this Pull Request:
This PR adds a default button height of
48px
to match the payment design guidelines and Woo defaults. If this height is not set explicitly on the container, the buttons default to40px
and look smaller than other buttons in the express checkout area. We also get the height from the newbuttonAttributes
API if available.Steps to test the changes in this Pull Request:
Changelog entry