-
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
Tabs: unify vertical tabs styles #65387
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: +37 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
@WordPress/gutenberg-design for now I've only removed style overrides. Here's the inserter > patterns Before (with overrides): After (without overrides): The main differences I spot in the previous style overrides vs default
For reference, here's the preferences dialog tabs, which already don't have any style overrides: These are the only two instances of Tabs at the moment in Gutenberg. Ideally, we'd find styles that work in both cases. If necessary, we can consider introducing some sort of variant, but let's try to keep that at a minimum and find something that we feel good about regarding future usage of the component, to prevent more style overrides in the future. Please leave some feedback, happy to implement any ideas so we can test them! |
+100 to removing the overrides. That said, I think the appearance in the Editor is the direction we want to go in. So would it make sense to update the component first, then remove the overrides after? That way there should be minimal visual disruption. One detail we need to consider is that the color change (gray or blue) is of inadequate contrast to meet requirements, so we need another way to communicate which tab is active. We could address this while updating the component. A good place to start could be making the active tab font weight slightly heavier. What do you think? |
Well, the thing is we only have two instances of vertical tabs, so what you're proposing doesn't make much sense purely in terms of the sequence of steps hehe. If, instead of removing overrides, I update the base component, then the preferences tabs will be affected, so either way there'd be visual disruption. Here's how I think this PR should evolve:
I hope that makes sense to you? Regarding your suggestion that the styles of the inserter tabs are closer to what we want, that makes sense to me. I'll tweak the base component, so both instances will look closer to it. In parallel, I will also apply your suggestion about the active tab styles. Then, we can take it from there. Sound good? :) |
@DaniGuardiola In my experience we don't tend to update multiple things in a single PR like you're suggesting, hence my idea to update the component first. That said, it does make sense to me in this context, providing other folks in the @WordPress/gutenberg-components team agree. |
…mprove/unify-vertical-tabs-styles
Currently, the weight for tabs is 500. In the inserter, it was overridden to 400. What if we:
This is how that looks like in inserter and preferences: font-weight.mp4I also tried making the active tab have the theme accent color (blue in this case) like the inserter style overrides did. For reference, this is the same color that tabs have when hovered. Here's how it looks like: active-color.mp4I quite like this with the light gray indicator. However, it might still not be enough for colorblind or low-vision people. Here, I tried making the active tab have a weight of 600 (on top of all previous changes): Kapture.2024-09-19.at.05.55.38.mp4And here's with 700: Kapture.2024-09-19.at.05.57.31.mp4I personally feel much more comfortable shipping this with the increased font weights. Thoughts? |
By the way, the font weight and text color changes also affect horizontal tabs, as you can see in the videos. That said, we could always special-case it to vertical tabs if we don't want that to happen. Maybe worth doing it in both for consistency though, it communicates more clearly that both (vertical and horizontal) are the same component rather than different ones. |
I reckon we should only apply the weight change to vertical tabs, if possible. Horizontal tabs are fine by virtue of the 'active' shape having enough contrast. In #64340 (comment) we're leaning towards Here's a visual for the different states, hopefully this communicates things better than words :D |
You can see this in my previous comment, it's the first example. Qs:
For now, I'm gonna assume that the answer is yes to points 1 and 2 and push those changes in this PR. FYI, there's a problem that #64371 fixes that makes the indicator not show up in the inserter, in case you wanted to try it locally. I'm performing a quick hack to get these screenshots and videos for now, but you won't be able to test it locally for now. Hopefully we'll be able to merge that soon though. Let me know if you want me to record or capture something in the meantime. |
I applied all changes, including the 400/500 weight change, and here's how that looks like currently. 500 (semibold) still seems too low for me, especially with the light blue indicator which looks even dimmer than the previous gray one to me. Kapture.2024-09-20.at.02.23.19.mp4 |
You may be right about the weight, but the more we increase it the more legibility in certain languages decreases. I think we should proceed with We can gather feedback and open a dedicated issue for this detail. There are other options to explore like adding a shape to the active tab: It's not something that needs to block the other enhancements here, and needs broader visibility to determine the best direction. |
One small detail; the radius on the active tab should be |
@jameskoster various thoughts:
|
@DaniGuardiola sorry, the border treatment wasn't a proposal; just an example of something we could try instead of changing the font weight. It's best to explore this in detail in a dedicated issue.
How bad are we talking? The majority of UI components have rounded corners, it should be a shame to introduce an inconsistency here. |
Sure, though I'm a bit worried that we'll make tabs worse if we merge this PR with a design that's considered "incomplete". The goal of this PR was to iterate on a unified design in the first place. I'm happy to split into multiple ones, but let's be careful not to make it worse before making it better.
Not that much. We just can't take advantage of Would you mind responding to the rest of the items in my last comment? I want to keep the PR moving forward :D |
I don't think so because we're objectively improving compared with trunk in terms of a11y.
Agree with you that this is okay for now.
Ideally the vertical tabs are 40px tall. This aligns with the general size convention we have for interactive elements (24/32/40). |
I find the light blue background harder to see than the previous gray one. This only affects the preferences tabs FWIW. That said, no strong feelings on my side, as long as we don't forget to improve a11y in a follow-up.
Cool!
Okay, right now they are 48px tall. I'll change this to 40px. For reference, this is the original size of the patterns/media tabs, ergo after this PR merges, the size change will only be noticeable on the preferences tabs. |
Here's a video taken in wp directly Kapture.2024-10-04.at.14.11.58.mp4 |
@ciampo I would like a new review as I updated a few things. I tested as much as I could and I think everything still works as expected, but I want to be sure. |
…mprove/unify-vertical-tabs-styles
Gave this a quick round of testing, and noticed some flashing styles for the focus ring in vertical orientation, when select on move is Apart from that, I couldn't spot other issues — although we'd definitely need to test thoroughly, as the devil's in the details. Finally, re. the e2e tests failures, it seems like it is a Safari quirk, and that the modal focus management is broken anyway. I'd be in favour of disabling that specific e2e test on Safari. |
Flaky tests detected in 07069c8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11183992724
|
[data-focus-visible], | ||
:hover | ||
) | ||
[role='tab']:is( [aria-selected='true'], ) | ||
& { | ||
transition: opacity 0.3s ease-in; |
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.
This works well enough so feel free to disregard this comment. It’s super subtle but I think a small delay is better. Mechanically the delay should match the transition duration of the indicator (0.2s
I think). However, to make sure it doesn’t feel like the icon appearance is late the delay can be less than that. Here’s my suggestion:
transition: opacity 0.15s 0.15s linear;
I also recommend linear
easing because on a quick opacity
transition ease-in
is imperceptible (though without the delay it might helpful in this case).
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.
We actually tried a few things including delay and different easings, and the current one felt decent. However if you've tested this value live and feel like it's an improvement, I'm happy to change it, since we don't feel too strongly about it anyway :)
Let me know! Really appreciate your input 👍
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! I did test but won’t argue it’s a significant improvement. It works as is. Non-linear easing on opacity or color transitions always invites my skepticism but in this case ease-in
is a pseudo delay.
I'm not seeing any issues in testing. I think I did see the flashing focus ring Marco mentioned but not now (since ece5efc). |
@stokesman yes, the flashing has indeed been fixed 👍 FYI, #65878 should be backported before this one to avoid nasty conflicts :) |
background-color: ${ COLORS.theme.gray[ 100 ] }; | ||
} | ||
&[data-select-on-move='true']:has( | ||
:is( :focus-visible, [data-focus-visible] ) |
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.
This is probably fixed by ariakit/ariakit#4094 which is included in @ariakit/react
0.4.11
.
We should follow-up to update ariakit-related dependencies, and consequently we should be able to remove :focus-visible
🤞
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.
Code changes LGTM, and test well in Storybook and in the editor 🚀
We'll just need to move the CHANGELOG entry under the Unreleased
section.
Hey folks, this is labeled as an enhancement, but it fixes a regression in 6.7. Therefore, it's unclear to me if this should be included in 6.7 itself. Thoughts? |
I thought the regression was introduced in #64371, which I thought was not part of 6.7 either — that's why this is marked only to be backported to Gutenberg RC. But if the fix is needed in 6.7, we should definitely backport it (together with #65837 and any dependant PR) |
You are correct, I just confirmed. I'll remove #65837 from the 6.7 board. Pardon the false alarm. |
Fixes #65837 (as a side effect)
Fixes #65863
Fixes #65865
What?
Unifies the styles of vertical tabs across Gutenberg.
Why?
See #64307
How?
By removing style overrides and tweaking the
Tabs
component to adapt to all needs.Follow-ups
UseAlready done!transform
Tabs: unify vertical tabs styles #65387 (comment)Testing Instructions
Try the inserter patterns/media vertical tabs.
Screenshots or screencast
d802-bb68-4bc8-a720-907961e44555.mp4