Components: Simplify MenuGroup component styles #65561
Merged
Conversation
This file contains 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
youknowriad
added
[Type] Enhancement
|
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. |
youknowriad
commented
Sep 23, 2024
| @@ -5,6 +5,10 @@ | |||
| .components-dropdown__content { | |||
| .components-popover__content { | |||
| padding: $grid-unit-10; | |||
|
|
|||
| &:has(.components-menu-group) { | |||
| padding: 0; | |||
There was a problem hiding this comment.
This is the main change, basically if a dropdown contains "menu groups", it shouldn't have padding. And all the magic CSS we had in place was just to account for that double padding.
There was a problem hiding this comment.
What if there is a mix of menu groups and simple items?
eg.
DropdownMenu
- Menu Item
- Menu Group
- Menu Item
- Menu Item
- Menu Item
In this case, wouldn't the first menu item be misaligned?
There was a problem hiding this comment.
The previous style were targeting specifically menu groups. In other words, if this exists somewhere, it's already broken. So I'm guessing it was "doing it wrong" anyway.
There was a problem hiding this comment.
For instance The following existing style wouldn't have worked in this case
&:first-child {
margin-top: -$grid-unit-10;
}
There was a problem hiding this comment.
I tweaked slightly the Storybook examples of Dropdown and DropdownMenu, adding a standalone <MenuItem /> next as a sibling to <MenuGroup />s
Click to expand diff
diff --git a/packages/components/src/dropdown-menu/stories/index.story.tsx b/packages/components/src/dropdown-menu/stories/index.story.tsx
index b6bc11ddec..dd4907bd0b 100644
--- a/packages/components/src/dropdown-menu/stories/index.story.tsx
+++ b/packages/components/src/dropdown-menu/stories/index.story.tsx
@@ -96,6 +96,9 @@ export const WithChildren: StoryObj< typeof DropdownMenu > = {
icon: more,
children: ( { onClose } ) => (
<>
+ <MenuItem icon={ arrowUp } onClick={ onClose }>
+ Standalone Item
+ </MenuItem>
<MenuGroup>
<MenuItem icon={ arrowUp } onClick={ onClose }>
Move Up
diff --git a/packages/components/src/dropdown/stories/index.story.tsx b/packages/components/src/dropdown/stories/index.story.tsx
index c6fe5014ff..0f07664787 100644
--- a/packages/components/src/dropdown/stories/index.story.tsx
+++ b/packages/components/src/dropdown/stories/index.story.tsx
@@ -99,6 +99,7 @@ export const WithMenuItems: StoryObj< typeof Dropdown > = {
...Default.args,
renderContent: () => (
<>
+ <MenuItem>Standalone Item</MenuItem>
<MenuGroup label="Group 1">
<MenuItem>Item 1</MenuItem>
<MenuItem>Item 2</MenuItem>
This scenario worked fine on trunk, but is broken in this PR:
| Before (trunk) | After (this PR) |
|---|---|
 |
 |
 |
 |
There was a problem hiding this comment.
Thanks for testing further. Personally, I still think that's "doing it wrong" but I pushed some CSS to fix it.
There was a problem hiding this comment.
Do you mean mixing standalone menu items with menu groups?
It may seem unusual (and we may discourage), but semantically it's valid and it's definitely an outcome of how consumers of the library can composite those components. Also, we may not have full control about this either in Gutenberg, depending on where the slotfills are.
IMO it's definitely a good thing to make sure this scenario looks and works as expected in any case.
youknowriad
force-pushed
the
update/simplify-menu-group-styles
branch
from
2db2e1a
to
15fb8dd
Compare
|
Size Change: +66 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 15fb8dd. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10991046477
|
ciampo
reviewed
Sep 23, 2024
ciampo
reviewed
Sep 23, 2024
There was a problem hiding this comment.
Left a couple of comments, but also realised that in theory there is already a check in place:
gutenberg/packages/components/src/menu-group/index.tsx
Lines 36 to 38 in c566725
Is that check not working as expected? If that check worked as expected, I guess there wouldn't be the need for any other changes?
|
Yes, the check doesn't work because of Slot/Fill. The slot is always rendered even if there's no fill, so basically children is always not empty. |
Got it. I guess the correct way would be for A potentially better compromise would be to set the Diff to apply to trunkdiff --git a/packages/components/src/menu-group/style.scss b/packages/components/src/menu-group/style.scss
index d9412c5049..c6119ec0ff 100644
--- a/packages/components/src/menu-group/style.scss
+++ b/packages/components/src/menu-group/style.scss
@@ -1,3 +1,7 @@
+.components-menu-group:has([role="group"]:empty) {
+ display: none;
+}
+
.components-menu-group + .components-menu-group {
margin-top: $grid-unit-10;
padding-top: $grid-unit-10;
|
This is what the PR is solving (see related comment in the other PR). For me the fix is simple enough that it's worth doing. Also this style is better and simpler to understand than the negative margins we have in trunk. |
ciampo
approved these changes
Sep 23, 2024
There was a problem hiding this comment.
This is what the PR is solving (see related comment in the other PR). For me the fix is simple enough that it's worth doing. Also this style is better and simpler to understand than the negative margins we have in trunk.
Yup! The only difference is that I was also ok with leaving the trailing extra margin, if that simplified the code changes / allowed us to merge the fix quicker. But I see that you pushed an updated version that doesn't break previous usages AND fixes the empty group case, so that's great!
andrewserong
approved these changes
Sep 24, 2024
There was a problem hiding this comment.
Thanks for the follow-up!
| Before | After |
|---|---|
 |
 |
I noticed that there are a couple of failing e2e tests related to block bindings. They appear unrelated to this PR as I notice they're failing on other PRs, too... not sure the culprit just yet, but I don't think it's a blocker to landing this one 👍
LGTM! 🚀
youknowriad
force-pushed
the
update/simplify-menu-group-styles
branch
from
9607ba8
to
599f7e1
Compare
youknowriad
force-pushed
the
update/simplify-menu-group-styles
branch
from
599f7e1
to
5859fb4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is meant to address this comment #65485 (comment)
What?
Basically, sometimes we have MenuGroup that are empty but because of how these components are styled, you end up with some weird spaces or separators when it happens.
The main issue is that we used to rely on
+CSS selectors to style multiples groups and consecutive groups. But the good news is I think we can simplify these styles. Initially we used this kind of styles because we didn't have the possibility to use:hasselector (it was not supported).Testing Instructions
Check the different dropdown menus: (block settings, more menu...) and ensure that spacing and separators all look correct.