I'm intrigued by the idea of navigateTo. In this example, its used to move the Navigation over, but not actually set the activeItem. I think we could do this internally if the item has children and change onClick to onSelect.

I kept navigation and selection separate as they are separate concerns imho.
My reasoning was that I don't see the use of activating the parent item, when navigating to a sub menu will hide the item.
I'm probably missing some context though. 🤔

and the implementation of the back button looks nasty.

Perhaps you're not finished with this just yet, but what advantage do you see by having each level designate its own back button?

Alternatively we can get rid of exposing NavigationBackButton altogether in favor of just having a prop parentLevel="root". Navigation2Level would be responsible for rendering the back button.

This points to a real challenge with this approach because the top level component won't know about its children. The initial slideOrigin won't necessarily be "left" because we may navigate from a link on the page that will represent an animation with slideOrigin "right".

Previously, we calculated a isNavigatingBack variable to solve this issue. Maybe Navigation analyses its children to determine hierarchy position?

Very good point!
I wanted to do something like isNavigatingBack but didn't have time to explore it.
Though I definitely didn't consider that the navigation could load "in media res", which requires a more involved handling of the slide origin default.

Actually, thinking about it, the initial slideOrigin could be omitted altogether, since it's not technically used (there is no animation on mount).

But regardless, with your implementation of NavigationLevel (I really shouldn't have used "Navigation2" as a name, now anything looks like "Navigation to level" 😄) the back button takes care of it automatically.

Previously, we calculated a isNavigatingBack variable to solve this issue. Maybe Navigation analyses its children to determine hierarchy position?

I like this idea. Though it would mean the children would have to be in the "correct" order to determine this.

Just thinking out loud here:

There are two kinds of navigation items at play: One that actually navigates and one that changes level. The first has no children and has setActiveItem in its onClick. The second has children, has navigateTo in its onClick and will render a right chevron. What if we separated them into distinct components?

<Navigation2Item
	slug="item-3"
	title="Item 3"
	onClick={ () => {
		setActiveItem( 'item-3' );
	} }
	activeItem={ activeItem }
/>
<Navigation2ParentItem
	slug="item-4"
	title="Item 4"
	navigateTo="nested-sub-menu"
/>

This allows Navigation2ParentItem to always render a chevron and have a clear set of props. It could be passed down in the render function so navigateTo can be part of its button's onClick.

Love this!
I might be inclined to have just one component with an additional prop indicating parent navigation, to avoid exposing too many components which I think could be cumbersome to learn and remember.

I also like this idea. It would be nice to expose the NavigationBackButton component for consumption leaving it with an onClick prop so we can resolve the back button not implemented in https://github.com/woocommerce/navigation/pull/68.

The initial activeLevel is going to be difficult to determine. If user goes to a url to a deeply nested nav item, the level will need to be determined somehow. I'll need to think on this one, but we may need to offload this logic to the consumer.

This is the only drawback I see to a more compositional route. We lose benefits for determining active state for levels and items. We won't be able to use the experimental route detection I was working on in #24985 which is okay, but as @psealock mentioned, this puts a lot of responsibility on the consumer to detect active levels and items.

Actually, thinking about this a bit more, if we do decide to go with route detection, we could simply pass down a method or prop or even determine this directly in the MenuItem. So that's a non-issue.

All that remains is to figure out how to set the active level/status on init and on change when many levels exist.

Ideally it would be sufficient to set the parentLevel slug and this can be inferred based on it, but I see why this duplication is needed with current approach.

Yup, the parentLevelTitle is unnecessary, and it might make more sense to call it backButtonLabel instead, since technically that's what it does.

Inferring it from the Navigation children is doable, but it would require a big enough change + possible discussion on the data handling, so I've delayed to a follow up. 🙂

This probably shouldn't fire if itemId is non-existent but we currently don't have a way to check that? (same for level below)

This makes total sense, but I'd delay to a follow up.

It would require the same children-traversing logic that we'd need to automatize the back button: doable but possibly long.

Yup, agreed that we can iterate on this later.

Noting here for a follow up PR because its the most visible place:

Checking presence of navigateToMenu for deciding if the item deserves a chevron or not presents awkwardness when programatically rendering NavigationItems. Consider looping through a bunch of items, some of which are links with hrefs, some are custom components, and others are items with children.

{ myItems.map( ( item ) => {
	return (
		<NavigationItem
			key={ item.id }
			item={ item.id }
			title={ item.title }
			href={ item.href }
			navigateToMenu={
				! item.href && ! item.customComponent && item.id
			}
		>
			{ item.customComponent ? item.customComponent : null }
		</NavigationItem>
	);
} ) }

The roundabout point I'm making is that it would be a nice-to-have if we do go the route of analyzing child elements that we could automate identifying which items have children and a chevron could be applied.

Thanks for pointing this out!

We should definitely make some of these props safer, and automatize some behaviours.
If an item has href it shouldn't "activate" the navigation behaviour, but I wouldn't prevent items with children/custom components from being navigation items (see for example the WP logo item in the story).

Either way, I've noted this down, and it will be among the many follow up issues I'm going to open after merging this PR. 🙂

What do you think about adding height: 100% here too? It seems the likely use cases would have it occupy the entire height.

I'm pondering a bit about how to tackle the style, especially in consideration of @ItsJonQ's comment.

What Q says is very valid: this component will be used in diverse ways, not just the dark-background left navigation sidebar that we are thinking of.

Maybe the 100% height (or the light/dark theme) should be the concern of the consumer?
Or maybe we could expose some preset props to make it easier? (e.g. a theme prop that can be light or dark, a fullHeight prop that makes it 100% tall)

I already have a style issue planned, and I'll add these concerns to that as well. 👍

A className would be helpful here. Otherwise, another height: 100% built in.

Whoops, experimental features shouldn't appear in the CHANGELOG:

To Project Contributors …
these APIs should neither be documented nor mentioned in any CHANGELOG.

I don't think this was correctly processed because it's under the Unreleased "version" but doesn't have a header like "Breaking Change" or "New Feature". This appears to have been released in 10.2.0 but has been stuck here…

cc: @gziolo

Yes, I think there is a bug in the script that processes changelogs. It's primitive so I'm not surprised 😂

In this case you are correct that it should not be listed although as far as I remember React includes experimental APIs as well. It's all subjective.

I opened #27436 that fixes the bug you discovered.

Thanks for taking care of this, I'll be more careful next time!