Unify focus styles, and pave the way for them to be customizable #21141
Conversation
jasmussen
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
jorgefilipecosta and
youknowriad
as code owners
jasmussen
added
[Type] Code Quality
| @mixin block-style__is-active() { | ||
| color: $white; | ||
| background: $dark-gray-primary; | ||
| @mixin input-style__focus() { |
There was a problem hiding this comment.
These mixins arguably should also be removed. However they are fairly widely used, and in places that are affected by upstream CSS bleed. That's why I omitted these for now. They can be revisited separately.
packages/base-styles/_variables.scss
Outdated
| $border-width-focus: 1.5px; | ||
| $border-width-tab: 4px; | ||
|
|
||
| :root { |
There was a problem hiding this comment.
Is there a better place than this SCSS file to output these? I imagine there is, because right now the variable appears duplicated a bunch of times, and it shouldn't be. I would appreciate advice here.
There was a problem hiding this comment.
What's the reasoning for using a CSS variable here. It seems inconsistent with the other variables (grids, colors...)
There was a problem hiding this comment.
They're both variables, that's all the reasoning there is. And that's why I was asking for advice on what might be a better place. It also gets duplicated when here, so it really isn't the best place — just not sure where to put it.
There was a problem hiding this comment.
I think the best way to do it would be something like
$border-width-focus: var( --border-width-focus, 1.5px );
and just use $border-width-focus everywhere without this root rule.
That said, I'd prefer if we avoid introducing these variables in this PR in order to give it more thoughts and also be consistent with the variables we propose.
There was a problem hiding this comment.
The reason for doing it the way it's been done currently, is to provide a fallback for IE11.
There was a problem hiding this comment.
It can potentially be automated using a Post CSS plugin. Maybe by replacing var above with a custom one config or something
There was a problem hiding this comment.
Good argument for leaving it to a separate PR.
|
Size Change: +13.2 kB (1%) Total Size: 879 kB
ℹ️ View Unchanged
|
jasmussen
force-pushed
the
try/tweak-focus-stroke
branch
from
1a22569
to
9948524
Compare
|
Okay, rebased and removed the CSS variable for now. I still think we should create a CSS variable for the focus style, so that it can be easily customized for example in options. But that is perhaps something for a different time. |
|
I tried this out today. In Chrome the focus states look wonderful. I did notice that the checkboxes and color circles still didn't reflect this new change.
and
But that can always be another PR. In Firefox however, the focus states seemed a bit awkward. They were even around the element. Some items showed a stronger bottom-right shadow, while others showed a thicker top border.
|
jasmussen
force-pushed
the
try/tweak-focus-stroke
branch
from
6d5ea59
to
a90ae14
Compare
|
Thank you Mark. I'm having a hard time reproducing the bottom right shadow issue in Firefox. The GIF you're showing has a thicker bottom border to indicate the active tab, this just happens to be the same color as the focus style. But I'm open to suggestions for how to tweak the details here — it feels important to keep the bottom border thickness even while showing focus. I did notice a little animation flicker that I'll see if I can address! |
|
Addressed the jumpiness. Forgot to mention that yes, good call on the color swatches and checkboxes. There are some elements inside the canvas as well, like when a social link button (not the block) has focus. But as you allude to, I'd like to revisit those separately as this PR is becoming a little big. |
jasmussen
force-pushed
the
try/tweak-focus-stroke
branch
from
32c45c9
to
5048298
Compare
jasmussen
force-pushed
the
try/tweak-focus-stroke
branch
from
5048298
to
4205f73
Compare
This PR does a number of things, the most important one perhaps just being a ton of code cleanup and simplification. It:
Additionally, it adds a new focus highlight CSS variable,— Out for now, can be revisited.--border-width-focus, which opens up the door to users customizing these.A couple of GIFs:
Here, a user customized the CSS variable in the inspector: