Navigation: menu links separator

[cpapazoglou]

Fixes #23293

Description

• adds a separator option ( None, / , • , |) for horizontal navigation menu blocks

How has this been tested?

Manual Test 1

• add a horizontal menu (with submenus)

Result

• you should be able to choose a separator between ( None, / , • , |) from the sidebar navigation block options
• choosing a separator should display it in the editor and in the frontend. Separators should display only for the first level navigation items (not in submenus)
• choosing None should not display any separator in the editor and in the frontend

Manual Test 2

• add a vertical menu (with submenus)

Result

• you should NOT be able to choose a separator from the sidebar navigation block options
• separators should not display in the editor and in the frontend

Screenshots

Editor	Frontend

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[shaunandrews]

This is a good start! A few things that I noticed now that I see this in action:

The dropdowns are showing the separators:

This brings up the question of how/if we support this in a vertical menu. In that scenario, the term "separator" doesn't really apply, its more of a "prefix" — and I'd expect it to attach to the first item so I could do something like this:

/ Home
/ About
/ Contact
/ Services

--

I also noticed that the separator now appears to affect the footprint of the block; That is, the block's size is affected by the separator, and causes some weird visual artifacts:

I'd expect the separator to appear outside the Link block's outline.

[cpapazoglou]

Thanks @shaunandrews for your prompt feedback! In my latest commit 738a4fb I have addressed the following:

• show separator panel only for horizontal navs
• show only for first level nav items

Regarding the footprint of the block, I believe it is something we have to live with?
As you can see below the li (green) contains it but the a (red) doesn't.

Since the separator is only decorative, I think we should not introduce an other intermediate li only for that purpose. In that case I would prefer styling the a (eg add a border) instead of li in my theme. It is exactly the same as having a separator in a breadcrumb.

[shaunandrews]

If we move ::before to wp-block-navigation-link__label, then we might be able to have better control over its placement within the block's foot print. I tried to hack it in, and kind of get it working, but then it breaks the data-separator.

[shaunandrews]

Regarding the footprint of the block, I believe it is something we have to live with?

I left a note about moving the pseudo-element from the a to the label itself, which I think would allow us to present the separator inside the block's footprint and give us more control of it's position without touch the block outline. It might also require some logic to apply a class to the Link or Navigation block when separators are present.

Since the separator is only decorative, I think we should not introduce an other intermediate li only for that purpose.

Totally agree.

--

I think this is a good start and I'd love to see it merged and followed up with some iterations around:

• The footprint stuff I mentioned above; Having the separator bump up against the edge of the block is a little sloppy.
• How themes can extend this; I don't see a way for a theme to provide a custom separator.
• Extending the UI to allow a custom separator; I bet people would love to drop some emoji in here.

[ZebulanStanphill]

I think adding this option is a bad idea. Can't this sort of thing be handled entirely by theme styles? And shouldn't it? It seems like a recipe for inconsistent design to allow the user to choose different separators for different Navigation blocks. That's the job of the theme (and possibly the upcoming Global Styles feature). I already think the option to show/hide the submenu indicator was a mistake. I really, really don't think we should be adding even more options like it.

[talldan]

Can't this sort of thing be handled entirely by theme styles? And shouldn't it?

@ZebulanStanphill Could you clarify what you mean by 'theme styles'? Not sure if you're referring to block style variations, or just the CSS added by a theme.

[cpapazoglou]

Thanks @ZebulanStanphill for chiming in

I think adding this option is a bad idea. Can't this sort of thing be handled entirely by theme styles? And shouldn't it? It seems like a recipe for inconsistent design to allow the user to choose different separators for different Navigation blocks. That's the job of the theme (and possibly the upcoming Global Styles feature). I already think the option to show/hide the submenu indicator was a mistake. I really, really don't think we should be adding even more options like it.

I totally agree with styling decisions being handled by the theme CSS. In that sense though, Typography shouldn't be an option also! It seems that we are trying to empower users that don't / can't write CSS.

For example, Typography would especially be very useful in cases that a translated horizontal menu gets too long and tends to overflow. Using a slightly smaller font-size would solve the problem.

Again, the separator seems to be a good addition to easily - fast show a separator, I would be very hesitant though to add styles configuration for it (padding, margin, size etc).

@shaunandrews thanks for coming back!
I would like to outline that separators should be in the after element so that when / if the menu items wrap the separator is shown at the end of the line and not in the start

Before	After

I left a note about moving the pseudo-element from the a to the label itself, which I think would allow us to present the separator inside the block's footprint and give us more control of it's position without touch the block outline. It might also require some logic to apply a class to the Link or Navigation block when separators are present.

Can you clarify what is the problem of the separator being part of the link block. Moving it to the label would:

• still make it part of the block
• will prepend the submenu icon
  
• will need extra styles (eg margin-left) and it will make the footprint of the block wider, thus possible breaking any existing implementations. In contrast to being just a character with no margin / padding which should / can be handled by the theme.

[ZebulanStanphill]

By theme styles, I literally mean styles provided by the current active theme.

In the case of Typography settings, there are legitimate use-cases for needing larger than normal text, e.g. a call-to-action. I do think the block editor has gone too far by providing a "Custom" option by default. It should only have provided the presets by default, with an admin setting somewhere to enable the "Custom" option.

If we want to "empower users" who can't write CSS, we should be doing that through the Global Styles project. One-off style tweaks are a design anti-pattern. We ought to be encouraging people to style things at a site-wide level and create/use reusable presets, not give them a band-aid solution to tweak one block here and another there... that's a foot-gun that ultimately results in an inconsistent mess. I know from experience, working on websites built with Divi Builder.

See also: #23893.

[talldan]

It's a good point about theme extensibility, @ZebulanStanphill.

Though at the same time I think it'd be less than ideal to lock a user into the single separator style provided by a theme's CSS.

Is there an incremental approach where we can use a hard coded list first and then hook it into global styles later? Who'd be a good reviewer in terms of global styles that can provide feedback on the PR? I personally don't know enough about it (on my list of things to improve my knowledge of).

[ZebulanStanphill]

I don't think we should expose this kind of feature at all until we can do so through Global Styles. I think it's a mistake to add stuff at the individual block level first, and then add it at a global level. That encourages one-off customizations in the meantime, and personally, I think some options should always be exclusive to the global and preset level.

Divi Builder, for example, added visual controls for pretty much every CSS property imaginable, but at the individual module level. So later, when they finally added a global defaults and global presets system, every website built prior would have a hard time adopting it, because everyone was already using one-off customizations on every page that would override the global settings. And furthermore, people weren't used to doing things at a global level, and the individual module settings were exposed more prominently, so even now, it's easy to fall into the trap of doing every as a one-off, because all the options are more prominent at the one-off level than the global level... when ideally, it should be the opposite.

In my opinion, style options should always be added at a global level first. Perhaps include the ability to create reusable presets for that option. And then, if that's still not enough, only then should you consider making it available on the individual block level. We should always encourage doing things the (usually) right way, and we should make that way more obvious than the (usually less ideal) alternatives.

Adding stuff in the reverse order is risky because it means we may end up removing options later on, which is annoying due to block deprecations, users who had grown accustomed to doing it one way, and having to continue to style existing blocks, bloating our stylesheets with legacy rules.

There's also something to be said for limiting access to style controls to certain user roles. Correct me if I'm wrong, but I think Global Styles is intended to be limited to higher-level WP roles, to prevent other users from messing up the website theme. I am not aware of any method to limit access to certain individual block controls by user role. There are things like the theme support flags (soon to be superseded by theme.json), but that requires a theme to specifically opt-out of providing controls in most cases. And of course, this opt-out applies to all users.

In short, I think it's premature to add something like this at the individual block level. I would, however, support the idea of adding this control at the Global Styles level. I also think we should look into adding the ability to easily create global presets through the Global Styles UI. I'm not sure whether these would be block style variations (which I know have some limitations) or something new.

[shaunandrews]

I think this adds a nice way to add some "flair" to your navigation without having to know CSS or switch themes. I'd expect there to be ways for theme's to opt-in/out and control the values. I also expect Global Styles to allow controlling the default settings for the Navigation block. My hope was to merge this and continue to iterate and integrate with the Global Styles work.

All that said, I do seem to be alone in this; I suggest we close this issue and move on. Maybe we'll come back to this further down the road.

[cpapazoglou]

I guess we should probably land it as Global Styles first and then check if there is a need for per block customisation. Till then, there are 1-2 easy "hacks" ( css - adding it as menu without link). Thanks for your feedback on this, I am closing this PR.