-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Vertical Tabs should be 40px min height #63446
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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 seems like the right place to fix this 👍🏻
[aria-orientation='vertical'] & { | ||
min-height: ${ space( | ||
10 | ||
) }; // Avoid fixed height to allow for long strings that go in multiple lines. | ||
} |
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.
Can we wrap this in a media query? For touch devices the larger 48px click area is helpful.
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.
For touch devices the larger 48px click area is helpful.
FWIW, the 40px size (the new default height for basic control components) was chosen specifically to be touch-friendly, so I don't think we need to upsize on touch devices.
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 added a breakpoint. Unfortunately we can't use the mixin in typescript.
Are we sure that makes sense in the general case? No strong opinion, but just wanted to make sure this decision wasn't colored too much by the patterns inserter case, where there happen to be a ton of tabs, and they are "secondary" to the horizontal tabs on top. (Or, it could mean that we can downsize the horizontal tabs to 40px as well.) |
Yes, I would think so. It's more about the target area and not the space between the words that you see. 40px is the standard large size that we should use in most cases. Horizontal tabs are fine as-is. |
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.
As I was looking through different instances of Tabs, I realized we have many divergences with custom styles. This was one of them, so I'd like to do a full pass and make sure we can minimize those divergences across instances. I might follow up with a PR that removes pretty much all custom styles so we can visualize it and make decisions about what styles/variants need to make it into the base component, or remain as a custom style. |
I second this. Ideally, we should try to generalize any solutions and have as few style overrides as possible. This will make maintenance easier and will ensure we're more consistent across the entire editor. |
Absolutely. When a different design from what the source component offers is needed, we should fall into one of these two buckets:
Keeping this mindset may cause slower pace of iteration, but it's immensely beneficial in the long term, both in terms of speed of iteration and consistency across the whole ecosystem. |
@ciampo is this clear to merge? |
I initially shared the same doubts as Lena, but if there's agreement that this change makes sense in the general case we can go ahead after adding a CHANGELOG entry (it can go under As a follow-up, I would also strongly recommend that we do a pass of all |
* Inserter: Reduce the height of the categories tabs to 40px * Make all vertically oriented tabs have a min height of 40px
What?
Vertical tabs should be 40px min-height.
Why?
We don't need as much spacing for vertical tabs.
How?
Apply
min-height: 40px
to tabs which have a[aria-orientation='vertical']
attribute.Screenshots or screencast