-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Try: 32px tall menu items. #73429
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
Try: 32px tall menu items. #73429
Conversation
|
Size Change: +129 B (+0.01%) Total Size: 2.58 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 6386cd4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19499319480
|
| align-items: center; | ||
| box-sizing: border-box; | ||
| padding: 6px 12px; | ||
| padding: 4px 12px; // Allows 32px min-height. |
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 can appear a sensitive change due to how high level and generic this is, so please give it any attention you feel is necessary. But I think it may not be as controversial after all. The height of the button is based on a default icon size of 24px, and then with padding to make the button have its previously-default height of 36px. The math checks out: 24 + 6 + 6 = 36. This is already paired with height: $button-size, where $button-size === 36.
Which means that even if we apply size="compact", which should make the button 32px tall, it still is 36px tall, because 24+6+6 forces it that way. By changing this padding to 4px, we now allow the button to be just 32px tall, but past default buttons should still be 36px tall because the $button-size still maintains that.
Let me know if this logic makes sense.
|
Denser menus seem worth trying. Should we update the newer |
andrewserong
left a comment
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.
|
It looks great. Nice improvement ✨ |
|
Not something to solve here, but since it could help inform our direction towards density as an aspect of theming (#73215) I'm curious about any expectations with regards to how |
|
I think there are valid use cases, but they are probably somewhat exotic at this point, so I'd veer towards solving that problem at a later time. And in the mean time just be aware, perhaps. I'm having to shift gears to some writing tasks at the moment, so it's unlikely I'll be able to address the good feedback already shared today. If you're antsy, feel free to push directly, otherwise I'll come back to this as soon as I can. |
|
I agree this is a good case for the new density theming. While |
|
That probably suggests the move toward @jasmussen I think we should update |
|
Okay, I believe I've updated the Menu component correctly now, but since that's using a particular setup, I would really appreciate some extra eyes on my last commit, and if something is off, feel free to push. Other than that, this one is ready for a final review, and then I can start updating changelogs. |
|
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. |
|
I have an ever so slight preference for what exists, but it's not at all a strong opinion, and if you prefer the rightmost one, feel free to push it. |
|
No preference really :) |
73d6544 to
198792c
Compare
|
Okay, I rebased, updated the snapshots and the changelog. Assuming things'll be green soon, would anyone like to give it a final check and perhaps a ✅? |
|
Maybe this could be the start of making everything mode condensed :P |




What?
Menus are getting slightly long as we are adding features:
Aside careful curation of items, as well as future grouping in flyouts, part of the reason they go long (and therefore occasionally have to scroll, or open upwards instead of downwards), is that all menu items are the default 40px size.
This PR proposes changing all menu items to be 32px by default.
Why?
It makes menu items far more compact, yet still comfortable in density.
Screenshots:
Testing Instructions
Check out the branch, then test: