-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Polish focus and active styles, and do cleanups #8385
Conversation
3, the darker border for input fields is very welcomed from an accessibility perspective, pending design feedback. In #7053 / #8037 a different solution is explored (darkening only the bottom border), which honestly I personally don't like that much. With regards to this PR, I have only one consideration so far: the comment is a bit misleading for two reasons: it's AA level and 3:1 is the minimum contrast for non-text. So it should be changed to: Worth reminding that for text contrast the minimum contrast ratio is 4.5:1 (with exceptions for large text) so the lightest gray that can be used is |
From a design perspective my only question is whether the border-radius should match for buttons and inputs, but overall the new design looks great. The darker border is actually kinda nice! 👍 |
Good catch Andrea, I'll revise the comment and add the other note as well.
This is a good catch. I'll noodle on this — the radius is the same as it is for the new buttons, but not for the old buttons. I'll see if I can unify all three somehow, or at least reduce to two. |
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 really like this and think it would be a benefit as is to get in. We can iterate from there. Thanks for working on this.
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.
Looks great. There are a few little tweaks worth making before merge. Just a few commented-out lines that should be removed and a few cleanup requests if you can make them.
After that, please do 🚢
@@ -166,15 +166,22 @@ | |||
|
|||
// Tabs, Inputs, Square buttons | |||
@mixin input-style__neutral() { | |||
outline-offset: -1px; | |||
//outline-offset: -1px; |
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.
🔥
} | ||
|
||
@mixin input-style__focus() { | ||
color: $dark-gray-900; | ||
outline: 1px solid $blue-medium-600; | ||
box-shadow: 0 0 0 2px $blue-medium-200; | ||
//outline: 1px solid $blue-medium-600; |
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.
🔥
border-color: $blue-medium-500; | ||
box-shadow: 0 0 0 1px $blue-medium-500; | ||
|
||
// Windows High Contrast mode will show this outline, but not the box-shadow |
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.
Comments should end with a period.
@@ -57,5 +57,5 @@ $block-parent-side-ui-clearance: $parent-block-padding - $block-padding; // spac | |||
$block-container-side-padding: $block-side-ui-width + $block-padding + 2 * $block-side-ui-clearance; | |||
|
|||
// Buttons & UI Widgets | |||
$button-style__radius-roundrect: 4px; | |||
$button-style__radius-round: 50%; | |||
$radius-roundrect: 4px; |
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 know it was like that when you got here, but since we're changing it anyway, could we make this $radius-round-rectangle
? roundrect
is quite unfriendly.
border-bottom: none; | ||
background-clip: padding-box; | ||
|
||
// put toolbar snugly to edge on mobile | ||
margin-left: -$block-padding - 1px; // stack borders | ||
margin-right: -$block-padding - 1px; | ||
margin-left: -$block-padding - $border-width; // stack borders |
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 a weird indentation and doesn't really quite explain this, but I think it's saying "This stacks the border on top of another border"?
I dunno, leave it if it's not clear, but since this line is already changing it'd be nice to tidy it up. 🤷♂️
2f61bb5
to
71ef5d4
Compare
Thanks for the reviews. Addressed feedback in 2f61bb5. Rebased and hoping the tests pass. Noting though, that this input field change also affects checkboxes: But I don't think that's a problem, quite the opposite actually. |
Last night tests were failing a bunch (as were local installs using |
It's legit, sort of: #8418. I'll see why the Docker container is messed up, but I'm not really sure why that's happening. |
This is sort of some cleanup work across the files.
9aaa0bc
to
5e9ef96
Compare
Rebased, squashed. Noting this causes a re-test. Hoping the tests pass now. |
Merging as tests pass and this is approved, yay! |
@ktmn I agree the select box could use some love. The answer is that yes, there's a reason. The reason is that it's so far the best input field design we've found that:
Gutenberg introduces a number of new design elements, and notably a number of accessibility improvements compared to WordPress core. We've tried a few other designs for input boxes that met these requirements, but so far the rounded version was the only one that passed the test for meeting all the requirements and notably feeling right in Gutenberg. We could try reducing the border radius to 3px, perhaps. This is the same radius that stock buttons use. |
I agree with @ktmn, having border radius on select, input, textarea fields just doesn't feel right. And there are other issues, inconsistencies:
|
A border radius even is small does have a softening effect I think is a positive impact visually. I am ok experimenting reducing, however I think overall it's a positive step. |
This PR does three things:
It replaces in a whole SLEW of files, the use of 1px for border widths, to use an SCSS variable. This is a cleanup job suggested by @tofumatt, and shouldn't affect anything. I don't expect we'll ever actually want to change the value of the $border-width, but by using a SCSS variable, we can make complex SCSS math easier to understand. For example 832a6c1#diff-b40a015680110faaabe748b80f5b4cc9L32 no longer needs an HTML comment.
It redesigns input fields. This is what needs design feedback. Before they were square and ligth gray, now they are rounded and darker, which fixes Some input boundaries lack sufficient contrast #7053. I also changed the focus style to be bolder — this may feel heavier at first hand, but with the darker base input style I felt like this was the only way to give contrast between base and focus style. This also fixes any issues with showing input fields on gray backgrounds, as noted in Some input boundaries lack sufficient contrast #7053 (comment).
At first glance these new styles might feel a bit too heavy handed, but after a little while the reduced amount of color in the focus style actually comes across as simpler. Give it a little while to work on you.