Size Change: +3.48 kB (0%)

Total Size: 1.07 MB

_{compressed-size-action}

Nice work. This is what I see:

There are some colors, metrics, sizes, tweaks, things to refine. I'll try and dive in and see if I can polish here.

It also looks like an animation has been replaced with a fade for the indicator, instead of lateral movement like here:

I'll see if I can restore that.

I pushed a few tweaks, now it looks like this:

We're getting closer here. One next step is fixing the focus style, which is supposed to be 1.5px admin-theme-color-blue. The CSS suggests this is accomplished by a border color and a .5px shadow:

But as you can see, the rule points to an object instead of a color. Here's the bit:

That "admin" color is defined here:

We need to revisit those admin color scheme treatments by the way, and ideally remove the fallback now that we don't have to support IE11. The colors are also wrong, where "medium blue" is not equivalent to the dark blue.

@ntsekouras do you know why that bit isn't working as it should?

@ntsekouras do you know why that bit isn't working as it should?

I guess it's okay to change to have the proper value like COLORS.admin.theme or even replace it with another value/prop entirely. It's definitely wrong as is.

Also I think it's okay to change to the proper color values in the COLORS.admin and remove the IE11 support too.

--cc @sarayourfriend

Note to self: if we remove the dark background color on the active button and rely on the backdrop, the horizontal sliding animation will be more visible.

Fixed sliding:

We'll still need to remove the !important for the active control. Without it, it's essentially minimal specificity competing against minimal specificity:

In vanilla CSS I'd have done this:

.ButtonView {
	color: black;
}
.ButtonView.buttonActive {
	color: white;
}

What's the CSS-in-JS equivalent?

We'll still need to remove the !important for the active control. Without it, it's essentially minimal specificity competing against minimal specificity:

I refactored not to use important here: b730335.

While this works, the approach I took was to use emotion css with classNames. What this does is combining all the styles from the classes passed, in the order they are passed. So we end up having a single css class with all the styles combined which means when isActive we have color twice but the active one takes precedence over the main prop. If any css-in-js more experienced people have a better alternative, I'm glad to change.

While this works, the approach I took was to use emotion css with classNames. What this does is combining all the styles from the classes passed, in the order they are passed. So we end up having a single css class with all the styles combined which means when isActive we have color twice but the active one takes precedence over the main prop. If any css-in-js more experienced people have a better alternative, I'm glad to change.

That's the exact right way to solve this problem, using the cx function! Great work!

Oh, there's another thing regarding the focus ring style I think we should improve. To me, it feels too subtle, and it's hard to notice when the focus is changing:

(yes, it's a GIF and I'm pressing tab)

What do you all think? I'm not sure how we're handling this in other components.

Oh, there's another thing regarding the focus ring style I think we should improve. To me, it feels too subtle, and it's hard to notice when the focus is changing:

I believe it's okay to land now as it's not been used somewhere yet. @jasmussen can take a look at it really soon, when he is back from his vacations.

Thank you all for the help, reviews and guidance! 🙇

(yes, it's a GIF and I'm pressing tab)

Oof this looks really bad. We should definitely address this. I'll add it to the list.

Thanks for landing this!

Congrats on landing this!

Oh, there's another thing regarding the focus ring style I think we should improve. To me, it feels too subtle, and it's hard to notice when the focus is changing:

I'll take a look at this one as soon as I can!