Gutenberg: Navigation Component: Improve navigateToMenu and Chevron Usage

[Copons]

Check if the NavigationItem defining navigateToMenu can actually be used to navigate to a hierarchical menu, and therefore needs a chevron.

https://github.com/WordPress/gutenberg/blob/668a710b3eb9c942b21eb5913fbe304b15ea71dd/packages/components/src/navigation/item.js#L66

Examples:

• If the item also defines a href it becomes an external link, which would clash with the internal navigation.
• If the item contains a custom component, it might be used to navigate to a menu, but the chevron might be in the way.

See https://github.com/WordPress/gutenberg/pull/25057#discussion_r486675148

[psealock]

Just a thought here. NavigationItem has this dual purpose, which is to render the navigateToMenu function and display child content such as links. Maybe it would make sense to split this into two components accordingly: NavigationItem and NavigationCategory? Something like that.

[Copons]

🤔 To be honest, I kinda think that a prop like navigateToMenu is specific enough in its intent, that it should have precedence over other props.

What would the "navigator item" component be like? How would it differ from NavigationItem?

• badge could make sense for both.
• item is the identifier, title the label: both components need both.
• navigateToMenu of course only for the navigator item.
• href of course only for the normal item.
• children? I'm still inclined to think that items with custom contents should be allowed to navigate.

They would probably end up being extremely similar, except for one prop, and for the presence of a chevron icon.

But regardless of the implementation: would separate components make the life easier for consumers?
This is what the Woo use case would look like:

{ items.map( item =>
  <NavigationItem href={ item.url } navigateToItem={ ! item.url && item.id } />
) }

{ items.map( item =>
  item.url
    ? <NavigationItem href={ item.url } />
    : <NavigationItemCategory navigateToItem={ item.id } />
) }

---

To be clear, my proposal for this would be to keep the one single component, and, if navigateToMenu is defined:

• Ignore href.
• If has custom content, maybe not render the chevron? (But we might figure out a way to make the chevron behave with the custom content).

[david-szabo97]

I think it would be difficult to come up with a good name for the second component. I don't think NavigationItemCategory is correct here. It's more like NavigationItemExternal and NavigationItemInternal but even that's hella confusing.

If we go for two different components, best would be for the NavigationItemCategory to just extend the NavigationItem, rather than two totally separate components.

const NavigationItemCategory = ({ ...props }) => <NavigationItem {...props} />

But after all, I agree with Jacopo, we shouldn't split the component into two.

[psealock]

Thanks for considering the possibilities, I think you all are right and its best to keep a single component.

If has custom content, maybe not render the chevron? (But we might figure out a way to make the chevron behave with the custom content).

Currently, the chevron is ignored with Custom content. Should we allow navigateToMenu items to have custom children? Seems like it may interfere with a uniform look and feel.

[psealock]

I'm cool to close this issue in this case, thanks to all for the input.

[david-szabo97]

Currently, the chevron is ignored with Custom content. Should we allow navigateToMenu items to have custom children? Seems like it may interfere with a uniform look and feel.

"It may interfere with a uniform look and feel" I agree, it would be great to be as flexible as possible. How about something like this:

1. If children is a valid React element then render as is
2. Else if children is a function then call it with { setActiveMenu } (exposing setActiveMenu function to the children)
3. Else render our default look

Edit: Removed code screenshots and created a PR instead https://github.com/WordPress/gutenberg/pull/25402

[Copons]

I'm personally a bit wary of rendering functions as they are often needlessly complicated to use. 🤔

To recap:

• Should navigateToMenu allow custom children? They would end up with an inconsistent look (e.g. no chevron).
• Should we add the onItemClick on custom children? It's not exactly cool from an a11y standpoint.

I agree about the inconsistent look, although I'm also inclined to say that a generic component should allow a certain degree of freedom.
navigateToMenu items have both a presentational (the chevron) and functional (the navigation) intent. Should they be tied to each other, or can they exist independently?

I'm inclined to think that we should allow navigateToMenu to contain custom children (without chevron), with the reasoning that... it's probably not a big deal. 😄
It's easy enough for us to develop, and it should be easy to consume as well.

NavigationItem example:

if ( ! children ) {
  return ( <Button onClick={ onItemClick } /> );
}

if ( navigateToMenu ) {
  return ( <div
    onClick={ onItemClick }
    onKeyDown={ event => {
      if ( ENTER === event.keyCode || SPACE === event.keyCode ) {
        onItemClick();
      }
  } }
    role="button"
    tabIndex="0"
  >{ children }</div> );
}

return children;

[david-szabo97]

I'm personally a bit wary of rendering functions as they are often needlessly complicated to use. 🤔

This would be an advanced usage of the component, so in this case I'd say it's fine. It's a very well known pattern IMO.
Take Animate component as an example.

We can also think about using a render prop instead:

<NavigationItem
    renderItem={ ( { navigateToMenu } ) => (
        <div onClick={ () => navigateToMenu( 'menu2' ) }>
            Custom
        </div>
    ) }
/>

[mattwiebe]

On the topic of accepting a function to render, it would seem to me that we only currently have theoretical use-cases at this point? If that is indeed the case, I would suggest not including it in the API surface for the time being.

It will be easy enough to add it later, if a compelling use-case presents itself, but difficult-to-impossible to remove should it turn out to have been a poor decision. And already knowing that it could be a source of visual confusion would make me very hesitant.

[Copons]

I agree with @mattwiebe.

What do y'all think of keeping the current behaviour and discuss it again if/when we can't avoid it?

To recap, the current behaviour is:

• navigateToMenu controls the navigation behaviour: adds a chevron to the item, and changes active menu on item click.
• navigateToMenu doesn't work with custom component items, but consumers can use the (optional) external state to change the active menu.

const [ activeMenu, setActiveMenu ] = useState( 'root' );

<Navigation activeMenu={ activeMenu } onActivateMenu={ setActiveMenu }>
    <NavigationMenu title="Home">
        <NavigationItem
            item="default-navigation"
            title="Default Navigation"
            navigateToMenu="sub-menu"
        />
        <NavigationItem item="custom-navigation">
            <Button onClick={ () => setActiveMenu( 'sub-menu' ) }>
                Custom Navigation
            </Button>
        </NavigationItem>
    </NavigationMenu>
</Navigation>

[david-szabo97]

Sounds good to me 👍

[psealock]

Yup, nice one here team. I'm happy with the conclusion you've come to that the current behaviour is sufficient and we can deal with a use case when it comes up. I'll close this and we can come back to it when the need arises.