Size Change: +1.89 kB (0%)

Total Size: 824 kB

_{compressed-size-action}

Hey @adamziel any idea why to me the menus are transparent?

Theme: Ambitious (blocks) and TwentyTwenty (classic)

@draganescu interesting! That background color should come from the following rule:

.is-style-light:not(.has-background) .wp-block-navigation__container {
	background-color: $light-style-sub-menu-background-color;
}

from navigation/style.scss.

Is it missing in your editor? Is the style variation not assigned maybe?

@adamziel the problem is that is-style-light does not apply by default for me and in any case the default should not be transparent ... right?

I'm also seeing the issue @draganescu mentioned in both Twenty Twenty and Gutenberg Starter Theme.

Other than that, the code LGTM and it's a big improvement when I test locally! Nice job 👍

Pinging @karmatosed for any feedback she has.

There's a bug report for the transparent submenus - #21449

I believe the problem with the background is that getBlocksWithDefaultStylesApplied only considers preferredStyleVariations and not the default block style. I'll proceed with this PR as it is and spin a new one to address/discuss it more.

I extracted the appender change into a separate PR (#22165).

The failing test here is also broken on master.

Let's move the discussion about the transparent background here: #22167

This is looking good! I saw one area that can be improved visually (excluding the glaring lack of background 😄 )

The top level of the 2nd subnav doesn't align to the top of the first subnav. I see that's because it aligns to the top of the focused/selected area of the parent nav item. This makes sense, but still looks odd.

Can we align the selected state AND the child level to the top on both accounts? Like this:

@karmatosed does this feel right to you? Or did you have some mockups closer to G2 that should be applied here?

@mapk in mocks there was a little gap, so I'd be keen to see that come back in. For example:

@adamziel I'm going to hold off another design review for now, once it's refined a bit I will give one again though. On the whole, this is shaping up really well and I am excited to see these iterations thank you.

I fixed the vertical alignment of sub-menus and merged this one. Let's do more improvements in another PR.

See #22228 and #22227

@karmatosed both PRs are now merged. What we did not reach a consensus on just yet, is whether or not the top-level navigation should have a background by default:

• Restore the missing background color on nested submenus #22228 (review)
• Fall back to default block styles when a new block is being inserted #22167 (comment)

Let's get it sorted out here and we should be good to marked this issue as resolved.

What we did not reach a consensus on just yet, is whether or not the top-level navigation should have a background by default:

@adamziel, no other text/link based blocks (ie. Heading, Paragraph, Media+Text, Quote, etc.) have a default background that I'm aware of. I think we're pretty safe following that pattern here. A setting that allows a background color to be added works well, but it's not necessary as a default.

@mapk I wonder if any other blocks have z-index stacked parts? Because that is the issue with no default background, see through things appear broken.

@draganescu, when a user adds a Nav block to their Header (in the near future), they'll likely add the Nav block on top of a solid color Header. It is also likely they'll want the background color of the Header to show through the Navigation block. I foresee this being a highly common experience based on web patterns out there.

Now, any elements that are z-indexed on top of the Nav block (ie. submenus) should definitely have a background color. I'm totally in favor of this, because it feels like this is the only time where we experience a z-indexing issue of content on top of other content.

Here's a quick diagram to help communicate my direction.

The user can always add a background color to the block with the Color Settings.

We should create an issue that explains this background setting behavior.