Change Navigation block markup on front end only #30551
Conversation
|
Size Change: +6.71 kB (+1%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
|
Thanks so much for trying this. This is what I see:
Outside of the little spacer complexity at the end (where the two first items should be ul wrapped, as should the last one, but not the spacer), from a pure user experience perspective, this feels like the right path forward. I understand the reluctance, but it is such a key part of the flow that it's crucial to get right. And this is a challenge that will not be unique to the navigation block, it will come up again and again. One incoming example is the table block receiving nesting support. Basic table markup looks like this: But when you add a header or footer, the markup changes: While the nesting system could replicate that 1:1, the user experience as it exists today, with each layer seletable, it would be unwieldy. This is an aspect where our friends at TinyMCE do it so well:
In other words, it's a problem we need to solve. In the case of the table block, a passthrough container block as discussed in #7694 might be a key ingredient in flattening the structure and showing a single block toolbar. In the case of the navigation block, the challenge is that the @youknowriad I'd love your thoughts here, as I know you've been looking at conditional divs, serverside rendered, lately. |
|
Coming back to this after thinking about it more, I think the most important bit of all is that we should not put the onus on the user to figure out what type of blocks need be wrapped in what containers. Users see a navigation block and assume they can insert links to pages, perhaps search, perhaps social links. When framed like that, what gets output on the frontend then becomes our headache to solve, either through server side rendering like in this PR, or revisiting the structure of the markup. With the server side rendering, the trade-off of markup that diverges between editor and frontend is easily worth it, as we can maintain similar classnames to target instead of the elements directly. |
|
I still feel like having unsemantic html on the editor side should not be used as an escape hatch. We should use this solution only if we arrive to the conclusion that it is impossible to have a good UX and semantic editor markup. For now it's just complicated to see the best solution but not impossible. |
There was a problem hiding this comment.
Great exploration @tellthemachines ! ✨
My overall impression is that we do want the overall behavior of this branch (in terms of user interactions), but two things might make us hesitate on this implementation approach:
- There's interest in solving this more generally for other wrapper blocks, as noted in the conversations from Blocks: Consider a "passthrough" supports for nested block wrappers #7694.
- This would be one of the first areas where we'd allow markup to intentionally diverge on the editor and may lead to some a11y usability issues
This definitely works, but if we end up choosing this approach, it'd be more of a necessary one-off since it's specific to the navigation block. We'd likely end up revisiting the problem for other wrapper blocks.
For next steps, I suspect we'll need to explore #30430, and if we hit a really bad blocker, we can double back to this approach
| if ($inner_block->name !== "core/navigation-link" && $inner_block->name !== "core/spacer" && $is_list_open === true) { | ||
| $is_list_open = false; | ||
| $inner_blocks_html .= '</ul>'; | ||
| } |
There was a problem hiding this comment.
Nice, much closer to expected html markup. If we go with this approach, children of ul, still need to be wrapped in an li. (For example the spacer div).
tellthemachines
force-pushed
the
try/nav-list-markup
branch
from
de31820 to
bdc5dab
Compare
|
The discussion in #31951 points to this solution as the best way forward, so I'm marking this PR ready for review now. I've also fixed some conflicts and updated this PR to work with the responsive wrapper. |
tellthemachines
force-pushed
the
try/nav-list-markup
branch
from
b176815 to
9661375
Compare
|
Updated this PR to also remove the I've also excluded the Spacer block from the Reviews and feedback appreciated! |
|
Took it for a spin:
As far as I can tell, this one threads the needle! Very impressive work 👌 |
| @@ -48,7 +48,7 @@ import { store as coreStore } from '@wordpress/core-data'; | |||
| import { ItemSubmenuIcon } from './icons'; | |||
| import { name } from './block.json'; | |||
|
|
|||
| const ALLOWED_BLOCKS = [ 'core/navigation-link', 'core/spacer' ]; | |||
| const ALLOWED_BLOCKS = [ 'core/navigation-link' ]; | |||
There was a problem hiding this comment.
Did we mean to remove the spacer from being insertable from submenus? We could maybe leave it for a follow up, it might be possible to do something like modify padding on the items instead of inserting an element for the published view.
As a note for the other reviewers, existing menus with a spacer still appear, but new menus won't allow spacers to be inserted in sub-menus.
There was a problem hiding this comment.
I removed it temporarily, as there are a couple of things we need to fix for it to work properly in submenus:
- the markup issue (ideally by getting the spacer to not render markup at all, or else by wrapping it in an
li) - making the spacer vertical within the submenu of a horizontal nav (right now it only works vertically within vertical nav blocks)
There was a problem hiding this comment.
At some "phase 2" of the block, we'll want to tackle mega-menus, where the contents of a dropdown are virtually as flexible as what you can insert in a group block, so it's something we should come back to. But definitely fine to wrap a solid V1 first.
|
Overall this looks pretty technically sound to me, and the user experience here feels good! I smoke tested a bit, and couldn't find any issues. In terms of moving this forward:
|
|
@gwwar what does this mean:
|
That we were okay with diverging editor markup from published markup. This is mostly to document for other 👀 since I don't expect folks to be aware of all the conversation threads here on it. |
annezazu
added
the
[Focus] Accessibility (a11y)
Description
Follow-up exploration into minimising impact of nav semantics for end users. This removes the
<ul>from the Navigation block markup and adds it conditionally on innerBlock type, only on the server-side render, so it only applies on the front end.Disadvantages: markup will be different between editor and front end;
editor markup will be unsemantic withUPDATE: this has now been fixed, to render all link children of the Nav as<li>elements nested directly inside the<nav>(we can fiddle with the Navigation link block and change its editor output too if we want to fix this).divs in the editor.Advantages: users don't have to bother about adding intermediary list blocks.
NOTE: Spacer blocks are (unsemantically) added inside theulelements for now, as I was given to understand that Spacers would be required to work inside submenus too. If this is not the case, I can remove them from the lists. Otherwise we'll need to think of another way to address the Spacer issue.The Spacer block has been temporarily removed from submenus and will be addressed separately; #33018 and #30590 explain what is needed.
With the changes in this PR, the Navigation block should contain fully semantic markup, no matter what its contents are.
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).