MenuItem: Add right padding for unchecked radio and checkbox items #34406
Conversation
andrewserong
added
[Type] Enhancement
andrewserong
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Nice work, this is what I see:
Those scrollbars, not sure why they are there. I suspect it's primarily something affecting the storybook rendering, though, so it may not need a fix. If it does need a fix, and doing a little inspecting, I noticed that something like the following would fix it up: |
Noting the same. @andrewserong, is there an easy way to check if the scrollbars would disappear when the On the other hand, I believe that @jasmussen 's suggested fix shouldn't hurt anyway. |
|
We'd probably want that width fix to be added to the popover component directly, though, in case that's what it takes. |
|
It appears the scrollbars is a regression in trunk as well 🙈
So it seems we need a fix. Whether slipstreamed in this one, or separately. |
|
Thanks for reviewing @jasmussen and @ciampo, I was originally working on another branch, so didn't notice the scrollbars when I moved the change to a fresh branch from trunk. It looks like the regression was introduced in #34378 — by reducing the padding on the div child of @jasmussen was the goal with #34378 to update all instances of the popover content to the smaller padding? If so, we can work through the list of instances of |
andrewserong
force-pushed
the
update/menu-item-component-checkbox-radio-item-padding
branch
from
53f66bb to
064736e
Compare
Just to add, for testing scrollbars within the editor, we should be able to do that with the Dimensions control on the Group block. The Reset all button receives the padding rule from this PR:
|
|
Size Change: +68 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
Yes, my mistake, I'll spin up a PR for that now, sorry about that! |
|
No worries, thanks for coming up with a quick fix! 🙇 |
andrewserong
force-pushed
the
update/menu-item-component-checkbox-radio-item-padding
branch
from
064736e to
a69ad33
Compare
|
I've just given this a rebase, and it appears to be working well now with no scrollbars. Let me know if you'd like any changes! |
There was a problem hiding this comment.
Looks good, nice work.
One thing I noticed is that now we've removed the min-width of popover menus, the tools panel is rather small. As we should for all popovers on a case by case basis, we can add back min-widths where it makes sense. I feel like to makes sense here, and that we should consider adding a min-width: 200px; to this particular dropdown. Feel free to do that here if you like, or we can follow up separately.
| &[role="menuitemradio"], | ||
| &[role="menuitemcheckbox"] { | ||
| .components-menu-item__item:only-child { | ||
| padding-right: $grid-unit-60; |
There was a problem hiding this comment.
Should we add a comment to denote what this particular value does?
|
Thanks folks, I've added in a comment explaining the reason for the padding. For the sake of neatness in the changelog, I'll add the minimum width for the ToolsPanel dropdown in a separate PR, and merge this one in shortly. |
andrewserong
force-pushed
the
update/menu-item-component-checkbox-radio-item-padding
branch
from
72cbe25 to
f5a19d5
Compare
|
The e2es were failing, preventing a merge, so I've just given this a rebase. |
andrewserong
deleted the
update/menu-item-component-checkbox-radio-item-padding
branch
I ran out of time to spin up a separate PR for that today, but will take a look next week if no-one beats me to it 🙂 |
Should we check and potentially add a |
I created #34532 which might provide a second layer fallback, which provides back a min-width if you use menu items inside. |
|
Thanks Aaron and Joen, between those two approaches, it looks like we're covered for the min-width 👍 |
The application of a |
|
Ah, of course, that makes sense, sounds good. |
|
Actually, don't mind me. If the ToolsPanel menu only had optional controls and none are selected then we could get inconsistent menu widths. So a min-width on the ToolsPanel menu probably still makes sense.
|
Description
Based on this suggestion, this is an alternative to #34375 with the goal of ensuring consistent width of the dropdown menu items between checked and unchecked states.
This PR adds right padding to checkbox and radio dropdown menu items that are only children — that is, they do not have icons or shortcuts.
The goal is to ensure that the padding is accounted for in the menu item's unchecked state, so that when it is checked, the menu item takes up the same amount of room visually. It also ensures that unchecked menu items do not encroach upon the space used by icons to denote checked status.
The expected behaviour is most notable with the ToolsPanel when we add a longer menu item to it, so this PR also includes an update to the ToolsPanel story, for testing. See screenshots below:
How has this been tested?
Manually using Storybook:
Go to http://localhost:50240/?path=/story/components-experimental-toolspanel--default and experiment with checking and unchecking the panel items.
Screenshots
With other items checked (note there isn't enough whitespace next to the third item):
With other items unchecked (note that in the before gif, the width of the popover is inconsistent between all items unchecked and one item checked)
Types of changes
(non-breaking change which fixes a styling issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).