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.

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

Try: Fix navigation gap & padding issues. #35752

Merged
merged 1 commit into from
Oct 20, 2021
Merged
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions packages/block-library/src/navigation/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@
.wp-block-navigation,
.wp-block-navigation .wp-block-page-list,
.wp-block-navigation__container {
gap: calc(var(--wp--style--block-gap, 2em) / 4) var(--wp--style--block-gap, 2em);
gap: var(--wp--style--block-gap, 2em);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aware this changes both the horizontal and vertical gap into a single gap property. But there didn't seem to be value in making this one be different.

}

// Menu items with background.
Expand All @@ -242,7 +242,7 @@
&,
.wp-block-navigation .wp-block-page-list,
.wp-block-navigation__container {
gap: calc(var(--wp--style--block-gap, 0) / 4) var(--wp--style--block-gap, 0.5em);
gap: var(--wp--style--block-gap, 0.5em);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why the fallback didn't work for me, but you can test on trunk: use empty theme, which doesn't supply a block gap value, then add a background color to the navigation block and observe how items collapse. Removing the calc and simplifying the selector seems to fix it for me.

Copy link
Member

@ramonjd ramonjd Oct 20, 2021

Choose a reason for hiding this comment

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

This is strange. I'm not sure why either, but when I don't use the shorthand definition, it looks as expected.

row-gap: calc(var(--wp--style--block-gap, 0) / 4);
column-gap: var(--wp--style--block-gap, 0.5em);

non-shorthand

With shorthand gap: var(--wp--style--block-gap, 0.5em);

shorthand

Having said that, I think having consistent gap/column values makes sense and I don't see any strange UI issues with the extra bit of row gap.

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Having said that, I think having consistent gap/column values makes sense and I don't see any strange UI issues with the extra bit of row gap.

Just an idle thought (and I'm sure there's a good reason not to do this), but I was wondering if since this block opts in to the line height support, whether we need to use row-gap? Could we set only column-gap and allow line-height to effectively control the vertical spacing between nav items?

Copy link
Member

Choose a reason for hiding this comment

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

Might be worth an experiment, thanks for the suggestion @andrewserong.

The Navigation Block does already opt-in to line-height.

I think the difference would be that the submenu text would also feel the effects of any line-height changes.

Fooling around with vertical spacing via line-height:
Screen Shot 2021-10-20 at 5 44 00 pm

With gap spacing only:

Screen Shot 2021-10-20 at 5 43 43 pm

And I think line-height is rather meant to define a font/text property? I'm no expert though and may be interpreting the specs incorrectly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an idle thought (and I'm sure there's a good reason not to do this), but I was wondering if since this block opts in to the line height support, whether we need to use row-gap? Could we set only column-gap and allow line-height to effectively control the vertical spacing between nav items?

I like to think that line height and gap can coexist, but also that for spacing things out, gap will be more intuitive. I also still have hopes that we can have the user surfaced block spacing control toggle between unified/axial, at which point you can tweak if you need to.

}
}

Expand All @@ -256,7 +256,7 @@

// When the menu has a background, items have paddings, reduce margins to compensate.
// Treat margins and paddings differently when the block has a background.
.wp-block-navigation:where(.has-background) a {
.wp-block-navigation:where(.has-background) .wp-block-navigation-item__content {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly a recent regression as of https://github.com/WordPress/gutenberg/pull/35716/files#diff-d66a4ed73d0d50a799547b78db235a4bad3428a0decb8895752032cd849d01f0R254, or the followup in https://github.com/WordPress/gutenberg/pull/35725/files#diff-d66a4ed73d0d50a799547b78db235a4bad3428a0decb8895752032cd849d01f0R46, but using the generic class (which we should've been using anyway) fixes an issue where has-background menu items had zero padding, when they should in fact have had some.

padding: 0.5em 1em;
}

Expand Down Expand Up @@ -465,7 +465,7 @@
}

// A default padding is added to submenu items. It's not appropriate inside the modal.
& :where(.wp-block-navigation__submenu-container) a {
a {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small fix for the overlay menu, to space things right.

padding: 0;
}

Expand Down