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: Non-link blocks should be wrapped in a <li> #23915

Closed
noisysocks opened this issue Jul 14, 2020 · 36 comments
Closed

Navigation: Non-link blocks should be wrapped in a <li> #23915

noisysocks opened this issue Jul 14, 2020 · 36 comments
Assignees
Labels
[Block] Navigation Affects the Navigation Block Needs Accessibility Feedback Need input from accessibility [Type] Bug An existing feature does not function as intended

Comments

@noisysocks
Copy link
Member

If you add a Search block into a Navigation block, the resultant markup is incorrect. The Search block should be rendered inside a <li>.

Screen Shot 2020-07-14 at 10 48 34

Steps to reproduce:

  1. Create a new post
  2. Insert a Navigation block
  3. Insert a Search block into the Navigation block
  4. Preview the post and inspect the markup

Expected result:

The <form> should be inside a <li>.

Actual result:

The <form> is inside the <ul>.

@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended [Block] Navigation Affects the Navigation Block [Feature] Navigation Screen labels Jul 14, 2020
@noisysocks noisysocks self-assigned this Jul 14, 2020
@noisysocks
Copy link
Member Author

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.

@youknowriad
Copy link
Contributor

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.

@ZebulanStanphill
Copy link
Member

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:

  • Navigation nav ul
    • Navigation Item li
      • Link a
      • now what?

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.

@tellthemachines
Copy link
Contributor

tellthemachines commented Jul 15, 2020

and blocks do not (to my knowledge) support returning a Fragment from edit().

Would it be possible to make them support returning Fragments 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.

@noisysocks
Copy link
Member Author

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!

@tellthemachines
Copy link
Contributor

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.

@ZebulanStanphill
Copy link
Member

Blocks outputting Fragments 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).

@talldan
Copy link
Contributor

talldan commented Jul 15, 2020

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:
Screenshot 2020-07-15 at 11 01 48 am

... but the markup is like this:
Screenshot 2020-07-15 at 11 00 03 am

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 #23745

@ZebulanStanphill
Copy link
Member

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)

  • Pages (for a dynamic list of top-level pages; top level element: ul)
  • Link List (for a customizable list of links; markup: <ul><InnerBlocks /></ul>)
    • Link (markup: <li><a>...</a></li>; essentially the same as it is now)
    • Text (markup: <li><span>...</span></li>; span may not be necessary; supports submenus just like the Link block)
  • Search (same block as outside of the Navigation block; same markup)

I really like this approach. It seems a lot simpler than anything suggested before.

@tellthemachines
Copy link
Contributor

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 🤷‍♀️

@ZebulanStanphill
Copy link
Member

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).

@talldan
Copy link
Contributor

talldan commented Jul 15, 2020

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.

@noisysocks
Copy link
Member Author

noisysocks commented Jul 15, 2020

Great discussion everyone.

Putting together all the threads above, I get a block hierarchy that looks like this:

  • Navigation: <nav><InnerBlocks allowedBlockTypes={ [ 'links', 'search', 'separator', 'pages', ... ] } /></nav>
  • Links: <ul><InnerBlocks allowedBlockTypes={ [ 'link' ] } /></ul>
  • Link: <li><a>...</a><InnerBlocks allowedBlockTypes={ [ 'links', 'pages' ] } /></li>
  • Search: <form>...</form>
  • Separator: <hr />
  • Pages: <ul>...(dynamic)...</ul>

In this model, Navigation definitely looks like a Group block with tagName set to 'nav'.

@tellthemachines
Copy link
Contributor

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.

@noisysocks noisysocks added the Needs Accessibility Feedback Need input from accessibility label Jul 15, 2020
@noisysocks
Copy link
Member Author

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>.

@draganescu
Copy link
Contributor

draganescu commented Jul 16, 2020

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.

@ZebulanStanphill
Copy link
Member

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

  • Archives
  • Categories
  • Pages
  • Custom Links
    • __ Link
      • Archives
      • Categories
      • Pages
      • Custom Links
        • __ Link
          • (and so on...)
  • Search

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

  • Custom Links
    • Custom Link
      • Pages
    • Page Link
      • Categories
  • Search

...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 divs.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Jul 16, 2020

Actually, thinking about it some more, since you could have multiple uls (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.

@noisysocks
Copy link
Member Author

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 #23745 (comment). It's worth exploring.


Two things that have been playing on my mind:

  1. 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...

  2. 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.

@tellthemachines
Copy link
Contributor

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

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 😄

@ZebulanStanphill
Copy link
Member

#24039 has been a great way to test and see the practical consequences of trying to use block context for situations like this. And after running into some issues with 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.

Quoting myself from that PR:

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 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 <InnerBlocks childWrapper="li" /> 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 <li><div>{ theChildBlock }</div></li>.

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.

@ZebulanStanphill
Copy link
Member

I went ahead and created an implementation of the aforementioned InnerBlocks enhancement idea: #24232. Let me know what you think.

@ZebulanStanphill
Copy link
Member

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.

@noisysocks
Copy link
Member Author

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.

@noisysocks
Copy link
Member Author

noisysocks commented Jul 30, 2020

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 (#23689), and "mega-menus" using the Columns block (#23745 (comment)).

I came up with two approaches:

  1. no-ul.html — which leans on Navigation being a specialised version of the Group block. Similar to Vimeo's.
  2. ul.html — which keeps the traditional <ul> and <li> elements. Similar to NIH's.

There's advantages to each:

  • no-ul.html
    • No need for an awkward intermediary Links block. Users can add a Link directly to a Navigation alongside a Search, Pages, etc. There's less potential for confusion about why one can't insert a particular block in a particular place.
    • CSS styling is really straightforward. The Navigation block is a container with flex-wrap: wrap and a user customisable flex-direction and justify-content.
    • The Pages block can have display: contents meaning that each page flows nicely with its parent's siblings. (This won't work in IE 11, though.)
  • ul.html
    • Using <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.
    • Potentially easier to store this in the database structure that the existing menus screen uses.

@draganescu
Copy link
Contributor

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.

@ZebulanStanphill
Copy link
Member

@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.

@ZebulanStanphill
Copy link
Member

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.

@ellatrix
Copy link
Member

__experimentalItemCallback={ ( item ) => <wrap>{ item }</wrap> }

@ellatrix
Copy link
Member

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
Copy link
Member

Cc @mcsf

@ZebulanStanphill
Copy link
Member

@ellatrix I like it! But how does it affect InnerBlocks.Content in the save implementation?

@noisysocks
Copy link
Member Author

Thanks for the discussion everyone!

Let's proceed with #23915 (comment). #24232 is a good start. In the medium term I'd like to explore #23915 (comment) 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 #24364.

@noisysocks
Copy link
Member Author

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?

@ellatrix
Copy link
Member

@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...

@ellatrix
Copy link
Member

@ZebulanStanphill We'll need to make a similar API for save. Perhaps getInnerBlocks and map it.

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 Needs Accessibility Feedback Need input from accessibility [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

7 participants