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

Fix Site editor navigation menu items alignment visual regression. #67321

Merged
merged 1 commit into from
Nov 28, 2024
Merged

Fix Site editor navigation menu items alignment visual regression. #67321

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 2 deletions packages/edit-site/src/components/sidebar-navigation-screen/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
.edit-site-sidebar-navigation-screen__content {
padding: 0 $grid-unit-20;

.edit-site-sidebar-navigation-screen-details-footer {
.components-item-group {
Copy link
Member

Choose a reason for hiding this comment

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

An alternative to targeting the inner component's class name, which could change without notice, would be to add a className to the <ItemGroup /> here:

gutenberg/packages/edit-site/src/components/sidebar-navigation-screen-main/index.js

Line 26 in 6e884dc

<ItemGroup>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but then it should be done for all the <ItemGroup>s in all the navigation sub-panels. Not sure it's in the scope of this PR though a follow-up for a better solution would be welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend avoiding selectors with the components- prefix whenever possible, otherwise this style may have unintended side effects if an ItemGroup component is added to the sidebar in the future.

This is the only place in all of Gutenberg where the .components-item-group selector is used. Admittedly, there are many other ItemGroup components that don't have CSS class names, but since they don't have their own styling, I don't think they need CSS class names in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@t-hamano we all agree CSS selectors from components should be avoided. However, this PR scope is to just fix a regression by restoring the previous usage where .components-item-group was already in use. We should avoid scope creep. Further improvements can be made in a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since what I proposed is just a small code quality improvement and it targets the same component, I think it's okay to include it in this PR.

Of course, we can address it in a follow-up, so feel free to merge it as is.

margin-left: -$grid-unit-20;
margin-right: -$grid-unit-20;
}
Expand Down Expand Up @@ -131,7 +131,7 @@
margin: $grid-unit-20 0 0;
border-top: 1px solid $gray-800;

.components-item-group {
.edit-site-sidebar-navigation-screen-details-footer {
margin-left: -$grid-unit-20;
margin-right: -$grid-unit-20;
}
Expand Down
Loading