ToolsPanel: Update unchecked menu items to use empty icon to ensure enough whitespace#34375
Closed
andrewserong wants to merge 1 commit into
Closed
ToolsPanel: Update unchecked menu items to use empty icon to ensure enough whitespace#34375andrewserong wants to merge 1 commit into
andrewserong wants to merge 1 commit into
Conversation
andrewserong
added
[Type] Enhancement
Contributor
Author
|
@aaronrobertshaw, you might have had another idea for how to handle this, so happy to go with another option if you have a preferred approach 🙂 |
|
Size Change: +13 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
Contributor
There was a problem hiding this comment.
Thanks for polishing this part of the ToolsPanel @andrewserong ✨
This works as advertised and is a nice touch.
I wonder about adding an empty SVG though to solely adjust something visually. I don't feel strongly, though I would probably lean towards just adding the following CSS rule to the ToolsPanelHeader definition in styles.js instead of the empty SVG approach.
.components-menu-item__button[aria-checked='false'] {
padding-right: ${ space( 14 ) };
}
The above CSS works for me in achieving the same result. What do you think?
Contributor
Author
|
Thanks Aaron! That’s a bit neater just adding in the CSS rule instead, sounds like a good approach, I’ll have a play with it tomorrow! |
Member
|
Definitely should not need an empty svg, but we should also not need custom CSS scoped to this specific use. The dropdown menu component should be doing the spacing by default, and any improvements needed should be done there directly. cc @ciampo @jasmussen |
Contributor
Author
|
Thanks for the feedback Matias! I'll take a look at the |
7 tasks
Contributor
Author
|
Thanks for the feedback, folks! I've implemented the CSS approach upstream in the MenuItem component and raised a separate PR, happy for more feedback over there: #34406 — I think it's a neater approach keeping within that component, but again let me know if anyone has other ideas. I'll close out this PR now. |
johnbillion
deleted the
update/tools-panel-menu-unchecked-items-to-use-empty-icon
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Noticed while testing #34065:
In the ToolsPanel component, if a longer ToolsPanelItem label is used and the item is unchecked, then its label encroaches upon the visual space that the check icon takes up in the menu.
In this change, we add an empty icon for unchecked menu items to ensure that the space surrounding the unchecked item is reserved for the presence (or in this case absence) of the icon. It also helps ensure that the width of the display options popover is the same when all items are unchecked as when they're all checked.
How has this been tested?
Via 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).