-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
Apply spacing improvements to the Checkout block #47565
base: trunk
Are you sure you want to change the base?
Conversation
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
Hi @nielslange, @nikkivias, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
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, @opr. I've successfully tested the PR based on your testing steps, so I'm pre-approving the PR.
Out of curiosity, is there a particular reason why in some cases you replaced $gap-larger
with ... rem
? Personally, I'd rather use $gap
and it's siblings, wherever possible. That said, spacing-wise, the PR looks great. I was just curious about the mixed units.
...s/woocommerce-blocks/assets/js/blocks/checkout/inner-blocks/checkout-terms-block/editor.scss
Show resolved
Hide resolved
left: -100vw; | ||
width: 200vw; |
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.
Nice approach! 🤩
94693ab
to
eedc710
Compare
@opr Thanks for your patience. I have gone through the different scenarios and things are looking good. Well done! And great workaround on the separator. |
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.
I have gone through the different scenarios and things are looking good. Well done! And great workaround on the separator.
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.
I've double-checked the PR and it works as expected. Both code and testing-steps are looking fine. Let's ⛴️ this improvement! 🙌
586263b
to
6d9c6cf
Compare
6d9c6cf
to
279b2c2
Compare
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR changes a lot of CSS to achieve the spacing requested in #46004 for both desktop and mobile.
Each checkout step now has a bottom margin of 48px. Previously, the titles of each step had padding/margin applied to the top however I moved it to the bottom of each checkout step.
Order notes, terms and conditions, and checkout actions all had to have some additional styling applied as they are outside of a "Checkout step".
As per the figma I moved the horizontal separator from between the Order Notes block and the Terms and Conditions block to be between the Terms and Conditions Block and the Checkout actions block (submit button). The separator is now tied to the checkout actions block and rendered above it.
Mobile
Desktop
Closes #46004 .
How to test the changes in this Pull Request:
Note
Please repeat these steps in Chrome/Firefox/Safari with varying screen sizes. Please also test in different browsers on your mobile. Please also test with different themes. I tested with Storefront, Tsubaki, Twenty Twenty-Four, Tove.
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Reference figma: 2JyXNimI4oGeJZhgQNZE9V-fi-2072%3A31996
Require checkbox
option.Changelog entry
Significance
Type
Message
Comment