Closed noisysocks closed 4 years ago
I'm stuck on how to approach this and would love some guidance from @WordPress/gutenberg-core.
The problem is that we want the Search block to render its contents within a <li>
when the Search block is inside a Navigation block but not when the Search block is inserted into a page, post, etc.
We will face the same issue if/when we try to support Social Links, Spacer, Separator, Paragraph, and Image inside Navigation. Additionally, should we want to make the Link block work outside of Navigation, we will run into the reverse of this issue which is that we don't want the <li>
to appear when rendering a Link in a page, post, etc.
Idea 1: Use block context
We could modify the Search, Social Links, etc. blocks to use block context to render a <li>
when one of the block's ancestors is a Navigation block.
A similar idea is to add a wrappingTagName
attribute to these blocks which is set e.g. using a block variation when the block is inserted into a Navigation.
Pros: Relatively simple. Cons: Potentially lots of duplicate logic. Requires blocks to "know" about each other.
Idea 2: Make InnerBlocks
support "wrapping"
We could modify InnerBlocks
to support a new e.g. wrapperTagName
prop which, when set, instructs the BlockList
component to wrap its children with <wrapperTagName>...</wrapperTagName>
. Navigation would then use <InnerBlocks wrapperTagName="li" />
.
One issue with this approach is that it potentially requires adding an "unnecessary" container <div>
to the Link block. This is because Links can contain sub-navigation <ul>
menus in addition to the <a>
, and blocks do not (to my knowledge) support returning a Fragment
from edit()
.
Pros: Relatively simple.
Cons: Requires container <div>
in Link. Adds extra complexity to the increasingly complex InnerBlocks
component.
Idea 3: Add a Navigation Item block, similar to the Column block
We could take a leaf from how Columns works and make it so that the only block allowed inside Navigation is Navigation Item. This would be a block that simply renders an <li>
and then its inner children.
Lots of care would need to be taken to make sure that insertion, block movement, and drag-and-drop within the Navigation block remain smooth. Potentially we could look at reviving the idea of supporting "passthrough" blocks so that the user isn't even aware that there is an intermediate Navigation Link block.
Pros: Potentially a complete solution. Cons: Relatively complex. Potentially confusing for users.
I'm hesitant between idea 2 or 3. I think I'd like to see 3 explored as the APIs are already here and see how it affects the experience.
Idea 1 sounds like a bad approach to me. Between the other two, I'm not sure which one I prefer.
The biggest issue I see with idea 3 is that it's not clear how it would actually work. So I guess you would have:
nav ul
li
a
The rest of the stuff in the Navigation Item should be in a ul
that's collapsed by default, but how do you pull that off in a way that isn't convoluted and hacky? It kinda sounds like the Navigation Item would have to have two InnerBlocks
areas, which is not currently possible.
Alternatively, the Link block would have to work more like it does right now, except with no outermost wrapping element... and that just brings us back to a similar issue as what we would run into with idea 2.
So it seems like our options are to either support multiple InnerBlocks
slots, or to add a wrapping div
to the Link block.
and blocks do not (to my knowledge) support returning a
Fragment
fromedit()
.
Would it be possible to make them support returning Fragment
s from edit
? That could be handy in other situations.
I'm more in favour of 2, mainly because I can't see how to make 3 work in a user-friendly way. With the columns block it's fine because the column
blocks are added automatically depending on the number you choose, but with the nav it doesn't seem right to make users pick a number of nav items, and having to add a nav item block only to then add another block inside it would be utterly terrible UX.
Edit: unless we could wrap the inserter in a nav item
, so that users wouldn't even know about the intermediate block. And then we'd have to also find a way to make it not show up when navigating between nesting levels.
Hm, that sounds complicated. Overall, I think having an extra wrapping div
somewhere is less costly than adding another level of structural complexity for users.
The rest of the stuff in the Navigation Item should be in a ul that's collapsed by default, but how do you pull that off in a way that isn't convoluted and hacky? It kinda sounds like the Navigation Item would have to have two InnerBlocks areas, which is not currently possible.
Nice catch @ZebulanStanphill, thanks for thinking this through.
It doesn't make sense for non-link blocks to contain submenu items as it isn't sensible for e.g. a user to hover over a Search block and see a submenu appear. Link is the only block that should support submenus. That means we don't have to support multiple InnerBlocks
in the Navigation Item block.
But you're totally right that that brings us back exactly to the problem in Idea 2 where Link has to either have its edit()
return a pointless wrapper <div>
or a Fragment
.
Would it be possible to make them support returning Fragments from edit? That could be handy in other situations.
Maybe! I'll do some investigation. I imagine it might break things like drag and drop, async rendering and Select mode. Also, where would things like the wp-block-*
class and data-block="*"
attribute go?
Edit: unless we could wrap the inserter in a nav item, so that users wouldn't even know about the intermediate block. And then we'd have to also find a way to make it not show up when navigating between nesting levels.
Funnily enough I was playing with exactly this idea over in try/add-navigation-item
!
Link is the only block that should support submenus.
We'll probably want some kind of non-link block, like Paragraph or Heading, to support submenus too, as that's a recurring pattern.
Blocks outputting Fragment
s in the editor would definitely break the block outlines shown when hovering the block icon or while using the Select tool.
...we don't have to support multiple InnerBlocks in the Navigation Item block.
This may not be true in the long run.
Something to keep in mind is the Pages child block proposal in #23689. This Pages block would likely be a child of the Navigation block (rather than a separate block), to allow for a Search block to be put into the same Navigation. I assume this block would consist of a series of <li>
elements (either in a Fragment
or a div
) put in the root level of the Navigation block.
In other words, if the Pages child block becomes a reality, the Navigation block would have to accept at least two different direct children: the Pages block and the Navigation Item block. That could make hiding the Navigation Item block a whole lot harder.
We'll probably want some kind of non-link block, like Paragraph or Heading, to support submenus too, as that's a recurring pattern.
Some kind of non-link text block that supports submenus would definitely be desirable. I don't think it makes sense to reuse the Paragraph block here, however. It's clunky to have a single p
nested in li
, and if we add submenu support to the Paragraph block, how do we disable it except in the context of the Navigation block?
I think instead there should just be a Navigation-specific child block for a text-only item (maybe just called Text). I guess its markup would be a span
element, which would allow it to be styled (assuming the Navigation Item block doesn't have styling controls; and if we want to hide it, it probably shouldn't).
Link is the only block that should support submenus.
Though at the same time, a Link block with a submenu only makes sense in the Navigation block. If a user were using a Link block elsewhere in a document I'd imagine it to be a single link.
Thinking about this differently, should the Search block actually be in the Navigation's <ul>
? I tried looking for a website that actually uses this pattern and couldn't find one.
The closest I could find was TechCrunch, which looks like this:
... but the markup is like this:
I'm thinking that we maybe want to consider a Link List block as the block that accepts Links, instead of treating the root navigation as a list. That may also get us closer to some of the options proposed on https://github.com/WordPress/gutenberg/issues/23745
If a user were using a Link block elsewhere in a document I'd imagine it to be a single link.
Just thought I'd point out that we already have a "single link" block: the Button block (which uses an a
element, not a button
). But yeah, as I pointed out in my previous post with the Paragraph vs Text thing, the Navigation (or Navigation Item) block's children that support submenus should probably only be available inside the Navigation block.
Anyway, great point about the Search block. With that in mind, we could have the block hierarchy work like this...
Navigation (consisting of simply the nav
when empty)
ul
)<ul><InnerBlocks /></ul>
)
<li><a>...</a></li>
; essentially the same as it is now)<li><span>...</span></li>
; span
may not be necessary; supports submenus just like the Link block)I really like this approach. It seems a lot simpler than anything suggested before.
I'm thinking that we maybe want to consider a Link List block as the block that accepts Links, instead of treating the root navigation as a list.
On the other hand, the HTML spec says
The nav element represents a section of a page that links to other pages or to parts within the page: a section with navigation links.
So perhaps the Search block shouldn't be inside the <nav>
tag at all. It could be that we're overthinking this and there's no real benefit to users from being able to add Search to the Nav block, vs., say, creating a Group block with a Nav and a Search inside it 🤷♀️
Both the W3C and WHATWG specs provide examples of other content in the nav
element, such as headings or even standard prose, pointing out that the nav
doesn't even have to use a list. So I think a Search block inside of a nav
is still technically valid.
That said, I don't know if there's much (if any) accessibility/semantic benefit to putting your search bar in a nav
. If there's no benefit, then you're right: it would be better to just use a Group block (or something similar).
Whether search belongs in nav does seem to be something rarely agreed upon. Lots of sites seem to have a search form inside a nav
, but then many don't.
I definitely think we would want to support multiple lists of links inside nav
, along with other blocks like Header, Separator, Spacer, Columns and probably Paragraph.
I feel like a Links block fits in quite nicely with the proposed Pages block too. A transform from Pages to Links could represent switching from an automatically generated list to a manually curated one.
Great discussion everyone.
Putting together all the threads above, I get a block hierarchy that looks like this:
<nav><InnerBlocks allowedBlockTypes={ [ 'links', 'search', 'separator', 'pages', ... ] } /></nav>
<ul><InnerBlocks allowedBlockTypes={ [ 'link' ] } /></ul>
<li><a>...</a><InnerBlocks allowedBlockTypes={ [ 'links', 'pages' ] } /></li>
<form>...</form>
<hr />
<ul>...(dynamic)...</ul>
In this model, Navigation definitely looks like a Group block with tagName
set to 'nav'
.
Whether search belongs in nav does seem to be something rarely agreed upon. Lots of sites seem to have a search form inside a
nav
, but then many don't.
Might be worth running this past the a11y team.
In any case, the only thing that changes is whether the search block goes in the Nav block or not; the hierarchy above would be pretty much the same either way.
If possible I'd prefer to have Navigation render the <nav>
. That way, the block editor in the Navigation screen can have templateLock: [ 'links', 'search', 'separator', 'pages', ... ]
(same as the Navigation block's InnerBlocks
) and the blocks inserted via this page can be spit out by wp_nav_menu()
which is usually already inside a <nav>
.
I've been musing for a while as well on inventing even more blocks to place inside a Navigation. NavBar (horizontal links), LinkList (vertical links) - these are variations of the same thing, or maybe Pages, or kinds of links PostLink, PageLink, CategoryLink, ExternalLink - there even is an issue to split Link or have it support a type and then do vatiations.
My problem was UX. So a user adds a Navigation block. Then they click the appender and the inserter contains a swath of options. That's a big departure from the origin of this block which enabled creation of simple navigation quite straightforward.
I don't think it's bad, tho. The block interface is basically nudging us to add as many blocks as needed to represent concepts on the canvas. It will always be easier to insert a CategoryLink block than a Link block, and then somehow find a category to associate. The same way, you may want to insert a CategoryLinks as a list of all or some categories, instead of manually picking all of them. Nevertheless, it's a rabbit hole.
I think that the Navigation block is a specialized container - like a special Group block that has so many particularities (e.g. mobile behavior, a different default layout based on variation etc.) that it deserves it's own inserter entry. It should render a div
by default because, by default, allowing images and text makes the initial state a "blank canvas", which is a div
.
Then the specialized innerBlocks, LinkList or Pages will render nav
elements, while Search will render form
, and they'll be siblings in the larger div
container, in an imaginary example menu that has [About] as a LinkList and [Services] as a Pages and then a Search. We can then optimize the generic case for situations when the div is superfluous.
Thinking of it, maybe that Submenu block, the one enabling mega menus, should be the Navigaition block itself and we allow recursive appending of this block?
In either case, on our road to visual website menu creation, we need to make sure that the user understands their power and also that they have a quick way out of complexity.
To allow a user to understand their power means we should drop the current UX of defaulting to a series of links, perhaps even dropping the idea that an empty Navigation should start with an empty link. If the user chooses to start from scratch that scratch should be a big plus sign, like @shaunandrews exemplified for the Submenu block.
The quick way out of complexity is found in the placeholder, the initial state and various toolbar options to create "simple" or "standard" menus, or by having more kinds of preset navigations in the inserter.
Some more thoughts about the Navigation-as-just-a-nav
idea:
If the Navigation block just became a nav
with InnerBlocks
, then we could let users insert the existing Archives or Categories blocks into it. The core or theme styles would handle the styling to make the ul
horizontal (if it's a horizontal Navigation). The proposed Pages block could also be made usable outside of the Navigation block the same way the Archives and Categories blocks are.
One thing to keep in mind, though, is that if you were to insert both a Pages and a Categories block into the Navigation, you would end up with this markup:
<nav>
<ul class="wp-block-pages">...</ul>
<ul class="wp-block-categories">...</ul>
</nav>
I think that's technically valid, but does it make sense? I guess from a semantic perspective, it does kinda make sense to split the Pages and Categories into separate lists. Depending on the CSS styling provided by the theme, they could still all be shown as a single horizontal (wrapping) list.
I do like the idea of separate blocks (or block variations) for page links and category links, plus a Custom Link block/variation for everything else.
Something my block nesting idea didn't account for was this: what if you wanted to have a dynamic page/category/etc. list as a submenu? To solve this, we simply have to change how the __ Link blocks work: their allowed children should be the same as the root Navigation.
So here's an example:
Navigation
The "__ Link" blocks would all have this kind of markup: <li><a>...</a><InnerBlocks /></li>
.
So for a block tree that looks like this:
Navigation
...the front-end markup would look something like this:
<nav class="wp-block-navigation">
<!-- InnerBlocks start -->
<ul class="wp-block-custom-links">
<!-- InnerBlocks start -->
<li class="wp-block-custom-link">
<a>Custom Link Label</a>
<!-- InnerBlocks start -->
<ul class="wp-block-pages">
(dynamic content)
</ul>
<!-- InnerBlocks end -->
</li>
<li class="wp-block-page-link">
<a>Page Title</a>
<!-- InnerBlocks start -->
<ul class="wp-block-categories">
(dynamic content)
</ul>
<!-- InnerBlocks end -->
</li>
<!-- InnerBlocks end -->
</ul>
<form class="wp-block-search">...</form>
<!-- InnerBlocks end -->
</nav>
I think this is the way to go. It requires no updates to InnerBlocks
, allows us to reuse existing blocks like Archives and Categories, and avoids using extra div
s.
Actually, thinking about it some more, since you could have multiple ul
s (not to mention potentially even a form
from a Search block) inside the InnerBlocks
slot of a "__ Link" block, we'll want to wrap the InnerBlocks
slot with a div
so that the entire submenu will be in a single container, making the CSS a lot simpler.
Thinking of it, maybe that Submenu block, the one enabling mega menus, should be the Navigaition block itself and we allow recursive appending of this block?
This is an interesting idea. I do really like the mockup @shaunandrews shared in https://github.com/WordPress/gutenberg/issues/23745#issuecomment-655771369. It's worth exploring.
Two things that have been playing on my mind:
It's difficult to make it so that non-link blocks like Search render outside of the <ul>
when using the Navigation screen. This is because the approach I took uses Walker_Nav_Menu::start_el()
to output a menu item's HTML content and this will be inside the <ul>
rendered by Walker_Nav_Menu::start_lvl()
. We may need to take a different approach to supporting non-link blocks in the Navigation screen...
I'll probably ignite a holy war by suggesting this 😅, but do we actually need to use <ul>
and <li>
? Doing away with them would let us make Navigation a regular container block and Link a regular block that can be used in other contexts. The HTML spec only says that <nav>
should contain <a>
links. I put together a <ul>
less Navigation in JSFiddle and VoiceOver seems happy with it.
It should render a
div
by default because, by default, allowing images and text makes the initial state a "blank canvas", which is adiv
.Then the specialized innerBlocks, LinkList or Pages will render
nav
elements
This approach would mean potentially having multiple <nav>
tags inside one Navigation block, which we should avoid for both semantic and accessibility reasons. The spec has a useful note on best practice for the <nav>
element.
I'll probably ignite a holy war by suggesting this 😅, but do we actually need to use
<ul>
and<li>
?
The short answer is no. The third example in the spec shows a <nav>
with only paragraphs and links in it. A list is a very common pattern though, because most menus are nothing more than a list of links. As far as I know, the advantage of a list accessibility-wise is having the number of menu items read out when the menu is entered, but that may not be useful in a complex menu with multiple lists and nesting levels.
So, we don't need to use a list pattern, but that doesn't mean we shouldn't if it helps keep back compat. I don't have a strong opinion on this though 😄
li
element wrappers and where to put the block CSS classes, I'm now convinced that the second idea suggested in this post is the right way to go.On first glace, I would think that, similar to how the "block alignment" wrapping `div` works (see the code a few lines earlier), the classes should still be applied to the `figure`, rather than the `li`. Additionally, I wonder what consequences this may have for styles targeting the `wp-block-image` class, since that could now apply to an `li` in some cases, rather than the `figure`. Either way, I suppose theme style issues are inevitable with this sort of massive change. Looking at the code, it looks like alignment wrappers are [specially handled](https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/block-list/block.js#L189) by the `BlockListBlock` component to ensure the editor-only `wp-block` CSS class is applied to the alignment wrapper and not the wrapped contents. Replicating the same behavior in this case would probably require a similar modification. But modifying that general component just for this one use-case would obviously be a hack. However, if we ended up taking the hypothetical `` kind of approach instead of using block context, the block classes could stay on the `figure` with no problems. Another point in favor of the `InnerBlocks`-prop approach: even if we generalize the block context name for this list-item wrapping, it's still left up to the individual blocks to implement the `li` wrapper, and it opens the door to inconsistent handling of where the block classes are put. Additionally, the `InnerBlocks`-prop approach would allow this child-wrapper behavior to be generalized even further to cases where the wrapper could even have multiple levels, e.g. `childWrapper={ MyWrapper }` where `MyWrapper` is a component that returns something like ` `. Another benefit would be that the child blocks would not need to be aware of the parent block to take advantage of this. The parent block would This would allow the Gallery block to more easily support 3rd-party image blocks without each of those blocks having to handle the `li` wrapping themselves. Yet another benefit: with block context, something like `isList` would be passed down to not only direct children, but also children-of-children. Since the direct children may not themselves be lists, those children would have to override `isList` themselves to prevent their children from inheriting the same value. Unlike something like `postId` or `postType`, the child block wrapper is only relevant to the parent and its direct children. The `InnerBlocks`-prop approach doesn't have this problem. Ultimately, the list-element wrapper feels more like a part of the parent, rather than the child. Though implementing an `InnerBlocks` `childWrapper` prop may result in a slight increase to the complexity of `InnerBlocks`, it would solve the issue in one spot and prevent other blocks from having to reimplement it over and over again, and it would remove the possibility for inconsistent handling of where the block classes get put. I wasn't really sure before, but now I'm convinced that the `InnerBlocks` approach is the way to go. { theChildBlock }
I went ahead and created an implementation of the aforementioned InnerBlocks
enhancement idea: #24232. Let me know what you think.
Clarifying my position on the InnerBlocks
child wrapper idea:
I definitely think it (or something similar) should most likely be implemented either way, and is probably the right approach for the Gallery block.
As for the Navigation block, however, there's an obvious issue with just modifying the current form of the block to wrap all its children in <li>
elements: that approach does not allow for things like the proposed Pages block, which have to be either a fragment of multiple <li>
s (kinda difficult to handle since there's no root element) or a <ul>
.
How I would expect the InnerBlocks
child wrapper prop to be used is not on the Navigation block itself, but on a child block called Custom Links (or something similar). Following my previous ideas, the Navigation would just be a <nav>
, with the dynamic Pages and the customizable Custom Links as the allowed child blocks. The Custom Links block would essentially just be a <ul>
that wraps all its children in <li>
s. In this approach, the Navigation Link block would most likely have to be modified to use a wrapping <div>
, since it couldn't continue to use a <li>
as a root element. However, the benefit of the child-wrapping approach is that all sorts of blocks could be inserted as children of the Custom Links block without having to be aware of lists, e.g. the Search block.
that approach does not allow for things like the proposed Pages block, which have to be either a fragment of multiple
<li>
s (kinda difficult to handle since there's no root element) or a<ul>
.
Pages is interesting. If edit()
and save()
can't return a Fragment then we'll end up with the same problem regardless of whether the Pages block's root element is a <ul>
or not: we likely want the children of the Pages block to appear visually alongside the siblings of the Pages block. Potentially this can be done by setting display: contents
on the root element, though that won't work in IE 11.
I realise this issue has grown in scope quite a lot. Thanks for coming along on the ride with me, it really helps to talk this stuff through! 😅
I took some time and tried to come up with HTML markup for the Navigation block that supports everything we want: static links, a Pages block (https://github.com/WordPress/gutenberg/issues/23689), and "mega-menus" using the Columns block (https://github.com/WordPress/gutenberg/issues/23745#issuecomment-655771369).
I came up with two approaches:
no-ul.html
— which leans on Navigation being a specialised version of the Group block. Similar to Vimeo's.ul.html
— which keeps the traditional <ul>
and <li>
elements. Similar to NIH's.There's advantages to each:
no-ul.html
flex-wrap: wrap
and a user customisable flex-direction
and justify-content
.display: contents
meaning that each page flows nicely with its parent's siblings. (This won't work in IE 11, though.)ul.html
<ul>
and <li>
means that screen readers will announce how many items there are in the menu.It's really common to see pattern around the web.~ EDIT: Using the list
and listitem
ARIA roles in no-ul.html
negates this. The only difference here now is that ul.html
looks better when unstyled.Lovely progress @noisysocks! There's two distinct issues we need to solve for:
1) How the navigation will render itself to be used in block based themes: this is where no-ul seems to bring more value. Is there some ARIA attribute that we could use somewhere to announce the number or items?
2) How the navigation editor renders classic menus plus blocks. This is basically unaffected on the front end by what we do here, as the classic navigation will go through a Walker - any theme can change defaults. We can go ahead and not change the default ul > li
which the walker uses to render items, with the caveat that blocks will always render adjacent to an UL.
@noisysocks For mega menus, wouldn't it make more sense to just use the CSS columns
property? That way, you can keep the list markup while still splitting it across multiple columns. It seems like a lot of people aren't even aware that CSS property exists, but it's perfect for this use-case.
Actually, on second thought, the CSS columns
approach would make sense for unsorted mega menus, but for ones intentionally divided with heading-ed columns, I guess using a Columns block would actually make sense in that situation.
__experimentalItemCallback={ ( item ) => <wrap>{ item }</wrap> }
My ideal future inner blocks API, which would be similar to the upcoming block API (#23034):
const { innerBlocks, innerBlocksWrapperProps } = useInnerBlocks( additionalProps );
<ul { ... innerBlocksWrapperProps }>
{ innerBlocks.map( ( block ) => <li>{ block }</li> ) }
</ul>
Having access to innerBlocks
also allow for an early return with 0 items.
Cc @mcsf
@ellatrix I like it! But how does it affect InnerBlocks.Content
in the save implementation?
Thanks for the discussion everyone!
Let's proceed with https://github.com/WordPress/gutenberg/issues/23915#issuecomment-667057204. https://github.com/WordPress/gutenberg/pull/24232 is a good start. In the medium term I'd like to explore https://github.com/WordPress/gutenberg/issues/23915#issuecomment-667059749 as it will let us have blocks like Pages inside a Navigation while still using <ul>
markup.
I wrote a high level overview of this feature and its next steps in https://github.com/WordPress/gutenberg/issues/24364.
My ideal future inner blocks API, which would be similar to the upcoming block API (#23034):
const { innerBlocks, innerBlocksWrapperProps } = useInnerBlocks( additionalProps ); <ul { ... innerBlocksWrapperProps }> { innerBlocks.map( ( block ) => <li>{ block }</li> ) } </ul>
Having access to
innerBlocks
also allow for an early return with 0 items.
@ellatrix: Any thoughts on how BlockListContext
and BlockListAppender
would fit into this API?
@noisysocks What is BlockListContext
? I only see it in native files. BlockListAppender
should ideally be removed in a refactor, but if it's something you want to keep for now you could render it separately...
@ZebulanStanphill We'll need to make a similar API for save. Perhaps getInnerBlocks
and map it.
If you add a Search block into a Navigation block, the resultant markup is incorrect. The Search block should be rendered inside a
<li>
.Steps to reproduce:
Expected result:
The
<form>
should be inside a<li>
.Actual result:
The
<form>
is inside the<ul>
.