-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Apply consistent font sizes and spacing in checkout (shipping and billing blocks) #59754
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
Conversation
@@ -85,7 +85,6 @@ export const CheckboxControl = forwardRef< | |||
<svg | |||
className="wc-block-components-checkbox__mark" | |||
aria-hidden="true" | |||
xmlns="http://www.w3.org/2000/svg" |
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.
Noticed it while doing style changes to the component. For inline SVGs in React this attribute is not needed.
&:only-child { | ||
margin-top: 1.5em; | ||
} |
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.
Went through all usages of the component; it is being displayed on its own only in a few places, and in all of them some additional styles are applied modifying the base margin.
It is also a safer default; the parent should be controlling the space around this component, not the component itself.
ed937f2
to
109fba0
Compare
b9b2d69
to
012b561
Compare
top: 1px + $gap-smaller; | ||
left: 1px + $gap-small; | ||
|
||
color: $universal-body-low-emphasis; |
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.
Aligned color with what we use for text input component
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.
Did a tiny refactoring of the component
@@ -79,20 +71,59 @@ | |||
} | |||
} | |||
|
|||
input[type="number"] { | |||
appearance: textfield; |
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.
Apart from moving a bit lower in the file I've added appearance
here. Commenting as it might be hard to spot in the diff.
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.
Scoped these styles properly; they were messing up checkboxes globally. A different question is why they were loaded on checkout, but I don't have the answer yet.
Testing GuidelinesHi @ralucaStan , Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
Size Change: -61 B (0%) Total Size: 5.91 MB |
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. |
This comment was marked as resolved.
This comment was marked as resolved.
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 tested this following the testing steps; Also tested on mobile Safari.
I found some visual bugs so I am requesting some changes. Outside of what I commented, I didn't find anything else.
...mmerce/client/blocks/assets/js/blocks/checkout/inner-blocks/checkout-fields-block/style.scss
Show resolved
Hide resolved
@@ -1,25 +1,28 @@ | |||
.wc-block-components-form .wc-block-components-text-input, | |||
.wc-block-components-text-input { | |||
position: relative; | |||
margin-top: $gap; |
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.
Not sure where this is coming from, but the Text inputs now have a content jump when I focus them.
Screen.Recording.2025-07-23.at.15.36.32.mov
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.
It's because I removed explicit height for both text-input and select components. Without it height is increased by 1px in the focused state because of the decision to use different border width there. There are more clever solutions for this, but I don't think this is the best PR to explore them.
I've brought back the height, this time with a pretty lengthy comment explaining why it's needed.
pointer-events: none; | ||
|
||
.has-dark-controls & { | ||
fill: $input-text-dark; | ||
} | ||
} | ||
|
||
> span, |
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.
do you remember why this was needed?
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.
Previously the height of the checkbox itself was not aligned with the height of the label, so there was a need for vertical alignment. Now, both the checkbox and the label have a height of 20px, so we don't need to do anything to get perfect vertical alignment.
a2d03c9
to
62d3f4b
Compare
579b69e
to
7ce46ee
Compare
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 can no longer replicate the bugs I mentioned.
Things work as expected.
I only wanted to mention this behaviour that is also on trunk: on input fields with value, when focusing, the content moves a bit horizontally.
If you'd have a quick fix for that it would be great to fix it as well in a followup PR.
I'm approving
I tried tracking this down, unsuccessfully yet. I will create a ticket in Linear within the project to take another look. Thanks for your attention to detail. |
0343ce3
into
straku/optimized-checkout-typography-and-spacing
Submission Review Guidelines:
Changes proposed in this Pull Request
Closes #59737
Continuation of work tracked in #59787 ("long" lived branch synced with trunk)
Intro post: pfHfG4-1iC-p2
Designs: fpl1YUEIW7suBFO09qymA2-fi-1212_50897
Covers shipping and billing blocks; they include a bunch of components used throughout checkout:
TextInput
(powers all of our form fields)Checkbox
Select
(powers country selector)Screenshots or screen recordings
Used the Twenty Twenty Four theme for the screenshots. Notice that in all cases the "After" screenshot has a lower height.
Address card
Shipping/billing section
All expanded
Mobile
Font size is increased on mobile to 16px to accommodate for mobile Safari auto-zooming on inputs with font size smaller than 16px.
How to test the changes in this Pull Request:
Note
If you want to quikcly spot which components were adjusted within this PR, you can go to global styles of the site and vastly increase the general size of the text in typography settings (Global Styles -> Typography -> Text -> Size -> XXL). Texts that will stay small after the change will be the one modified here, the rest is coming in follow-up PRs.
Testing that has already taken place:
The above.
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment
Targeting a different branch than
trunk
. The final PR will include changelog.