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

Navigation: Consider a different markup structure for submenus. #39392

Open
scruffian opened this issue Mar 11, 2022 · 8 comments
Open

Navigation: Consider a different markup structure for submenus. #39392

scruffian opened this issue Mar 11, 2022 · 8 comments
Labels
[Block] Navigation Affects the Navigation Block [Type] Enhancement A suggestion for improvement.

Comments

@scruffian
Copy link
Contributor

At the moment nested navigation items generate markup like this:

<!-- wp:navigation -->
	<!-- wp:navigation-submenu -->
		<!-- wp:navigation-submenu -->
	 		<!-- wp:navigation-link /-->
		<!-- /wp:navigation-submenu -->
	<!-- /wp:navigation-submenu -->
<!-- wp:navigation -->

This structure outputs markup like this:

<ul>
	<li class="wp-block-navigation-submenu">
		<a></a>
		<ul>
			<li  class="wp-block-navigation-submenu">
				<a></a>
				<ul>
					<li>
						<a></a>
					</li>
				</ul>
			</li>
		</ul>
	</li>
</ul>

Now if I want to target the dropdown portions of the navigation block using theme.json I might define something like this:

"core/navigation-submenu": {
	"typography": {
		"fontSize": "24px"
	}
}

This will generate CSS like this:

.wp-block-navigation-submenu { font-size: 24px; }

This problem is this CSS targets the navigation item above the drop down as well as the items within it, which is unexpected, and makes it impossible to target just the dropdown on its own.

To overcome this I am proposing we use this structure for the navigation:

<!-- wp:navigation -->
	<!-- wp:navigation-link -->
		<!-- wp:navigation-submenu -->
			<!-- wp:navigation-link -->
				<!-- wp:navigation-submenu -->
	 				<!-- wp:navigation-link /-->
				<!-- /wp:navigation-submenu -->
			<!-- /wp:navigation-link -->
		<!-- /wp:navigation-submenu -->
	<!-- /wp:navigation-link -->
<!-- wp:navigation -->

Which would output markup like this:

<ul>
	<li>
		<a></a>
		<ul class="wp-block-navigation-submenu">
			<li >
				<a></a>
				<ul class="wp-block-navigation-submenu">
					<li>
						<a></a>
					</li>
				</ul>
			</li>
		</ul>
	</li>
</ul>

This allows us to effectively target dropdown using theme.json.

Does this create other problems? Are there other ways to solve this?

Originally noticed in #39277

@Humanify-nl
Copy link

I hope this proposal may also open up the possibility of using the submenu container for colors, instead of placing these styling classes (has-background etc.) on the links inside it.

@tellthemachines
Copy link
Contributor

Currently the Submenu block contains its parent element, because that element is not always a link. Depending on the "open on click" and "show arrow" settings, it may display a button, a link or a link with a button. This is the main reason we're not using the Nav Link block for the parent; you can find more context in #33775.

Does this create other problems?

If I understand correctly, the proposal is making the Submenu an inner block of a Nav Link block. That's not straightforward because, as mentioned above, the parent isn't always a Link. We'd have to either introduce some very hacky logic in the Link block to treat it as a button depending on whether it has children, or we'd need another Link-or-Button-depending type of block, which also doesn't feel ideal.

In terms of styling, we may also want to target the parent element, which will be harder to do if it's not part of the submenu.

Are there other ways to solve this?

Current default targetting of the block wrapper for styles (whether they come from the editor or theme.json) doesn't work for all cases; we had similar problems with the Navigation block itself when implementing layout on it - see #37473. Table is another challenging block in this respect - see #30791. This is a problem that won't always be possible to solve by fiddling with the block markup; we need to have a way to target its inner elements. Part of the Style Engine work will be looking at ways of doing this (#38167) - contributions to those discussions very welcome! 🙂

@scruffian
Copy link
Contributor Author

You're proposing that we use something like __experimentalSelector to modify the color properties of theme.json to target only those items within the UL? That makes sense to me :)

@tellthemachines
Copy link
Contributor

If it's likely that themes will only want to style the items within the ul, then yes, __experimentalSelector works!

@Humanify-nl
Copy link

If I understand correctly, the proposal is making the Submenu an inner block of a Nav Link block. That's not straightforward because, as mentioned above, the parent isn't always a Link. We'd have to either introduce some very hacky logic in the Link block to treat it as a button depending on whether it has children, or we'd need another Link-or-Button-depending type of block, which also doesn't feel ideal.

I understand this is why removing a submenu block, also removes the parent link. Which is intuitively unexpected behavior. Or is that another issue?

@tellthemachines
Copy link
Contributor

I understand this is why removing a submenu block, also removes the parent link. Which is intuitively unexpected behavior. Or is that another issue?

That is the expected behaviour, yes. If you wish to remove the dropdown while preserving the parent link, you can remove the submenu items, and transform the submenu into a link.

As I mentioned above, there's an open on click setting on the Navigation block whereby submenu top-level items become buttons. In that context, it doesn't make sense to preserve them once the submenu is removed.

@Humanify-nl
Copy link

That is the expected behaviour, yes. If you wish to remove the dropdown while preserving the parent link, you can remove the submenu items, and transform the submenu into a link.

I understand why it works like this. Yet, from a user psychology perspective it is not intuitive because:

image

Here it says "add submenu". Which implies a menu is added to a link (but it actually is a transform). A user would expect that when removing the submenu, the same revert action applies (the submenu is removed, but actually transformed to a link).

But it does not. It simply removes. You have to have prerequisite knowledge about the transform but a new users doesn't 'intuitively know' we transformed in the first step so we assume we don't have to.

See how this simple signalling of "add" instead of "transform" can be confusing for someone who has never used the menu?

@tellthemachines
Copy link
Contributor

Here it says "add submenu". Which implies a menu is added to a link (but it actually is a transform). A user would expect that when removing the submenu, the same revert action applies

Good point! Let's change the copy to say "Transform to submenu".

@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

4 participants