Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Components: Simplify MenuGroup component styles #65561

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Components: Simplify MenuGroup component styles #65561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d22276
Components: Simplify MenuGroup component styles
youknowriad Sep 23, 2024
262beb4
Update Changelog
youknowriad Sep 23, 2024
5859fb4
Handle standalone items use-case
youknowriad Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
- `ToolsPanel`: atomic one-step state update when (un)registering panels ([#65564](https://github.com/WordPress/gutenberg/pull/65564)).
- `Navigator`: fix `isInitial` logic ([#65527](https://github.com/WordPress/gutenberg/pull/65527)).

### Enhancements

- `MenuGroup`: Simplify the MenuGroup styles within dropdown menus. ([#65561](https://github.com/WordPress/gutenberg/pull/65561)).

## 28.8.0 (2024-09-19)

### Bug Fixes
Expand Down
3 changes: 3 additions & 0 deletions packages/components/src/dropdown-menu/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/dropdown/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
23 changes: 10 additions & 13 deletions packages/components/src/dropdown/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,16 @@
.components-dropdown__content {
.components-popover__content {
padding: $grid-unit-10;

&:has(.components-menu-group) {
padding: 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance The following existing style wouldn't have worked in this case

		&:first-child {
			margin-top: -$grid-unit-10;
		}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Screenshot 2024-09-23 at 15 42 16 Screenshot 2024-09-23 at 15 40 58
Screenshot 2024-09-23 at 15 42 02 Screenshot 2024-09-23 at 15 41 10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing further. Personally, I still think that's "doing it wrong" but I pushed some CSS to fix it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


.components-dropdown-menu__menu > .components-menu-item__button,
> .components-menu-item__button {
margin: $grid-unit-10;
width: auto;
}
}
}

[role="menuitem"] {
Expand All @@ -13,22 +23,9 @@

.components-menu-group {
padding: $grid-unit-10;
margin-top: 0;
margin-bottom: 0;
margin-left: -$grid-unit-10;
margin-right: -$grid-unit-10;

&:first-child {
margin-top: -$grid-unit-10;
}

&:last-child {
margin-bottom: -$grid-unit-10;
}
}

.components-menu-group + .components-menu-group {
margin-top: 0;
border-top: $border-width solid $gray-400;
padding: $grid-unit-10;
}
Expand Down
5 changes: 4 additions & 1 deletion packages/components/src/menu-group/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
.components-menu-group + .components-menu-group {
margin-top: $grid-unit-10;
padding-top: $grid-unit-10;
border-top: $border-width solid $gray-900;

Expand All @@ -10,6 +9,10 @@
}
}

.components-menu-group:has(> div:empty) {
ciampo marked this conversation as resolved.
Show resolved Hide resolved
display: none;
}

.components-menu-group__label {
padding: 0 $grid-unit-10;
margin-top: $grid-unit-05;
Expand Down
Loading