Navigation Component: Composition Proposal #25057
Conversation
* Initial working tree display * add styles * add styles * dark theme * back button * secondary * BEM style classnames * remove back button styles * add menu: 'primary' data propery * get even more working * remove extra styles * add README * manifest
* Break nav components into smaller pieces * Get visible menu items based on menu id and active state * Allow null and root text in back button * Allow root text for navigation title * Add line break to make use of multiple menus more obvious * Fix back button case at root level * Remove secondary nav styling * Simplify component props * Pass parsed data back to navigation children * Move nav menu item logic into menu * Simplify nav components to bare essentials * Allow menu level navigation without changing active item * Handle PR feedback
* Migrate styles to css-in-js * Add spacing to navigation child items * Remove opinionated styling * Rename styling components
* Migrate styles to css-in-js * Add spacing to navigation child items * Add badge property and styles to navigation items * Update UI component naming * newLine Co-authored-by: Paul Sealock <[email protected]>
* Allow custom link tag and props for menu items * Let Button handle logic and add LinkComponent * Add back in link tag const to allow sharing of props Co-authored-by: Paul Sealock <[email protected]>
* Navigation Component: Remove setActiveLevel child function arg (#24704) * remove setActiveLevel from public api * change to NavigationBackButton * return null for backButton if no parentLevel * Navigation Component: Expose __experimentalNavigation component (#24706) * Expose __experimentalNavigation component * fix typo * expose menut and menu-item as well * Loop over navigation levels and wrap with animate * Use map and set to organize items and levels * Simplify level and item logic * Remove unnecessary key prop * Fix parent level assignment * Add back in key prop to force animation * Use useMemo to map item data Co-authored-by: Paul Sealock <[email protected]>
* Update readme * first attempt to define data * Update packages/components/src/navigation/README.md Co-authored-by: Joshua T Flowers <[email protected]> * Update packages/components/src/navigation/README.md Co-authored-by: Joshua T Flowers <[email protected]> * better declaration * Add onClick prop * note parent default * Update packages/components/src/navigation/README.md Co-authored-by: Joshua T Flowers <[email protected]> Co-authored-by: Joshua T Flowers <[email protected]>
* Don't trigger onClick prop when not provided * Add class names for external styling
* Add navigation component tests * Test cleanup * Add link and custom component tests * href should be undefined, not null Co-authored-by: Paul Sealock <[email protected]>
* apply effect on activeItem change * Update packages/components/src/navigation/stories/index.js Co-authored-by: Joshua T Flowers <[email protected]> Co-authored-by: Joshua T Flowers <[email protected]>
* don't animate on mount * better way using useRef * fix * reverse isMounted:
Copons
added
[Type] Experimental
|
Size Change: +1.52 kB (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
* Update nav styles to match new core designs * Add menu title prop and menu title styling * Add dark styling * Add secondary menu to nav story * Add tests for menu titles * Fix back button style component name
|
cc @joshuatf |
There was a problem hiding this comment.
Thanks for getting this going @Copons. I made some initial thoughts and observations, but I'll keep playing with the idea. I think a disadvantage of the compositional approach is the lack of central knowledge of the children and hierarchy, but maybe we can overcome this.
Thinking about extensibility, the compositional approach shifts the extension mechanism from filtering data arrays to Slot/Fill which can be handled completely by the consumer. From what I understand, this is a direction we'd like to go in anyways.
| title="Item 3" | ||
| onClick={ () => { | ||
| setActiveItem( 'item-3' ); | ||
| } } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 🤔
| <NavigationBackButton | ||
| parentLevel="sub-menu" | ||
| label="Back to Sub Menu" | ||
| /> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -121,6 +121,96 @@ const Navigation = ( { activeItemId, children, data, rootTitle } ) => { | |||
| ); | |||
| }; | |||
|
|
|||
| export function Navigation2( { children } ) { | |||
| const [ activeLevel, setActiveLevel ] = useState( 'root' ); | |||
| const [ slideOrigin, setSlideOrigin ] = useState( 'left' ); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| </MenuItemUI> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -121,6 +121,96 @@ const Navigation = ( { activeItemId, children, data, rootTitle } ) => { | |||
| ); | |||
| }; | |||
|
|
|||
| export function Navigation2( { children } ) { | |||
| const [ activeLevel, setActiveLevel ] = useState( 'root' ); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Thank you so much for the in-depth reviews @psealock and @jeyip! ❤️
Good catch! I've removed the absolute position, so now it renders just below the menu. Being outside of the dark background should make it clear enough that it's not part of the Navigation component. I've addressed the suggestions:
Suggestions to be figured out in follow ups: |
There was a problem hiding this comment.
Overall this looks great to me @Copons! ✨ I left some non-blocking comments and questions.
We could maybe add a PoC in storybook that showcases how changes in navigation state can affect things outside of it. This is currently reflected in example state, but I thought it might be nice to add some visual indicator for the example.
|
|
||
| The parent level slug; used by nested levels to indicate their parent level. | ||
|
|
||
| ### `parentLevelTitle` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 🙂
|
|
||
| const activateItem = ( itemId ) => { | ||
| setItem( itemId ); | ||
| setActiveItem( itemId ); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yup, agreed that we can iterate on this later.
|
As @vindl pointed out, the name I've renamed "level" as "menu", which in my opinion makes things clear. Additional changes since the approvals:
Even if there have been lots of changes, the behaviour of the component hasn't changed at all. |
|
Haiii! Wow! Looks like a lot of things have been changed! Apologies for delays. I've been away for most of this week! I looked through the code and the examples. I think the updated mechanics address some of my concerns from the initial implementation. The NavigationContext should come in handy for my intricate UI (e.g. dynamic header title/resize based on current path) 👍 I do think that the Navigation should have be less coupled with UI though, as we may need to use it in other places - for example the right sidebar (FSE/Global Styles): The component API/setup should feel similar to the example left-hand Navigation, but the UI can be rendered completely differently - ideally composed with components vs. CSS overrides. Those are just my thoughts though! |
| </ItemBadgeUI> | ||
| ) } | ||
|
|
||
| { navigateToMenu && <Icon icon={ chevronRight } /> } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 🙂
| color: #f0f0f0; | ||
| padding: 8px; | ||
| overflow: hidden; | ||
| `; |
There was a problem hiding this comment.
What do you think about adding height: 100% here too? It seems the likely use cases would have it occupy the entire height.
There was a problem hiding this comment.
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. 👍
| className={ classnames( { | ||
| [ animateClassName ]: | ||
| isMounted.current && slideOrigin, | ||
| } ) } |
There was a problem hiding this comment.
A className would be helpful here. Otherwise, another height: 100% built in.
Hi @ItsJonQ welcome back! Good reminder that this component is supposed to be used in various places! I think right now the only thing that would get in the way with the Global Styles example is the dark theme (I'll open a design issue about it as mentioned above). I'm looking forward to discuss and tackle all these concerns in follow ups. ✨ |
|
Pinging @afercia to check this out for an accessibility review. |
Hi @ZebulanStanphill, I've opened an issue to discuss the Navigation component's accessibility: #25259 🙂 |
| @@ -2,6 +2,8 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| - Introduce `Navigation` component as `__experimentalNavigation` for displaying a hierarchy of items. | |||
There was a problem hiding this comment.
Whoops, experimental features shouldn't appear in the CHANGELOG:
To Project Contributors …
these APIs should neither be documented nor mentioned in any CHANGELOG.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I opened #27436 that fixes the bug you discovered.
There was a problem hiding this comment.
Thanks for taking care of this, I'll be more careful next time!
Description
Expose a
<Navigation />component from@wordpress/componentsto render menus in a hierarchal fashion.This component will be useful in the Full Site Editing Sidebar project and WooCommerce's Navigation project.
How has this been tested?
This has been tested using Storybook and unit tests. In order to test the component, run the following:
Next, go to Components > Navigation and see the component in use.
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: