Navigation Component: Merge feature branch #24920
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 <psealock@gmail.com>
* 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 <psealock@gmail.com>
* 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 <psealock@gmail.com>
* Update readme * first attempt to define data * Update packages/components/src/navigation/README.md Co-authored-by: Joshua T Flowers <joshuatf@gmail.com> * Update packages/components/src/navigation/README.md Co-authored-by: Joshua T Flowers <joshuatf@gmail.com> * better declaration * Add onClick prop * note parent default * Update packages/components/src/navigation/README.md Co-authored-by: Joshua T Flowers <joshuatf@gmail.com> Co-authored-by: Joshua T Flowers <joshuatf@gmail.com>
* 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 <psealock@gmail.com>
|
Size Change: +9.63 kB (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I've got two minor complaints and one comment, but otherwise this works as expected!
I think this component can be a solid foundation to be improved upon by integrating style and features suggested, for example, by @ItsJonQ in ItsJonQ/g2#20.
|
|
||
| export default Navigation; | ||
| export { default as NavigationMenu } from './menu'; | ||
| export { default as NavigationMenuItem } from './menu-item'; |
There was a problem hiding this comment.
Not a super fan of having default and named exports together, but I'd argue this is fine since we would end up importing from the package index anyway. 🤷
There was a problem hiding this comment.
Agree. I'm following a pattern already in place though:
gutenberg/packages/components/src/tree-grid/index.js
Lines 161 to 163 in 5897bfa
There was a problem hiding this comment.
Ha! I've tried opening a bunch of random index.js files and never found the same pattern, but I suspected it was there somewhere. 😄
|
Thanks for your continued efforts on all! I checked out the code and I poked around with the Storybook example. It looks like the (infinite?) nesting feature is now working :D, which is great! The menu items are tabbable and can be "clicked" with keyboard presses, which is nice. A couple of observations from my end: Needs to render more than just linksOne of FSE's sidebar designs features a UI where the elements that appear within the menu are not just links. Instead it's a searchable list (of pages). This particular view is a nested one (maybe level 2 or 3?) At the moment, it appears that nested level renderings are handled via Below is a GIF prototyping that interaction:
It comes from a G2 Components storybook prototype: FSE appears to be taking advantage of the new Sidebar designs. The Sub-Nav Rendering LifecyclesContinuing with the note above (on making sub-nav content more flexible)... For example, the FSE Template mock (above) may require fetching/loading of templates via A simpler example would perhaps be async fetching of badge counts. (Avoid) Animation on mountThe menu slides on initial mount. Either the Hope this feedback helps! |
@ItsJonQ It is currently possible to render anything in a list with the Right now you can see it used with a custom Button in the storybook: const CustomRouterLink = ( { children, onClick } ) => {
return <Button onClick={ onClick }>{ children }</Button>;
};
// ...
const data = {
title: 'Internal link',
id: 'item-5',
LinkComponent: CustomRouterLink,
};Nothing (visual glitches notwhitstanding) stops us from rendering something else in there: const CustomImageLink = ( { onClick } ) => {
return ( <a onClick={ onClick }>
<img src="page-template.jpg" />
</a> );
}Regarding the search. Using that new "SearchTitle" component could be opt-in, so if you don't need it you can keep using |
|
Thanks for the review @ItsJonQ !
As noted above, there is the ability to pass in custom components, complete with their own lifecycle hooks.
Interesting use case, thanks for noting. This would make sense for paginated data. I'm fairly sure the composibility affords this possibility. I'm wondering if a more involved Storybook exploration makes sense here. It may be a good idea for demonstrating a search input as well.
Excellent point. Here is an issue to track that task: #24950
Indeed: #24951 |
|
Another thing that comes to mind, when thinking about customizability, is that we can extend the For example, in #23939 (comment), there are two interesting proposal for the FSE navigation: groups and previews on hover. The simplest example could be having a
I guess what I have in mind is something similar to the block type definition, with all those |
|
Thanks for the thoughtful feedback @Copons !
This is the strategy employed on the WooCommerce side: A
Because we opted to provide a minimalist solution, it made sense to offload that logic to the consumer. If this is a feature required by FSE as well, it could pay to investigate a solution where sections are dictated in a more formal way, such as via the data array or perhaps something else entirely.
Interesting idea here. Although the
Nice! This exploration would lend itself well to a follow up PR |
* apply effect on activeItem change * Update packages/components/src/navigation/stories/index.js Co-authored-by: Joshua T Flowers <joshuatf@gmail.com> Co-authored-by: Joshua T Flowers <joshuatf@gmail.com>
|
I've noticed a bit of a odd behaviour with e652c32. It might not be a big deal, but let me point it out anyway. The Back button doesn't update the active item. It's noticeable with the new "non-navigation link" which only shows up if the active item is not I wonder if it would make sense for the Back button to also clear the active item. E.g. // stories/index.js
const [ active, setActive ] = useState( 'item-1' );
return (
<NavigationBackButton setActive={ setActive }>
<Icon icon={ arrowLeft } />
{ parentLevel.title }
</NavigationBackButton>
);
// navigation/index.js
const NavigationBackButton = ( { children: backButtonChildren, setActive } ) => {
if ( ! parentLevel ) {
return null;
}
const onClick = () => {
setActiveLevelId( parentLevel.id );
setActive( undefined );
};
return (
<Button
className="components-navigation__back-button"
isPrimary
onClick={ onClick }
>
{ backButtonChildren }
</Button>
);
}; |
|
Haii. Wanted to give a heads up. It looks like Global Styles designs is exploring the potential of having nested navigation as well, but for the right sidebar:
Figma: https://www.figma.com/file/oEkcAyhIvPFMVEAO8EImvA/Global-Styles?node-id=421%3A4508 I did an early prototype here: Ideally, this Navigation component/system should be flexible enough to create this right sidebar/nav experience as well, since the mechanics are very similar. |
@psealock It would be awesome to see this solution tackling more complex cases 💪 . Inspiration/use-case examples may be tricky to draw from as FSE + Global Styles designs are still in flux. |
* don't animate on mount * better way using useRef * fix * reverse isMounted:
This is by design. Previous designs called for navigation to a set of child items (forward or backward) to set the active item as the first item, but this has been abandoned so that users can navigate the navigation without actually changing the page. Only when an item is clicked does the active item change and page navigation occurs.
Right, another way to look at it is you'd be on the "Child 2" page already, so the link wouldn't be there until you navigated away. |
| @@ -67,6 +67,14 @@ const Navigation = ( { activeItemId, children, data, rootTitle } ) => { | |||
| } | |||
| }, [ activeItem ] ); | |||
|
|
|||
| const isMounted = useRef( false ); | |||
There was a problem hiding this comment.
I think this doesn't need an initial value, e.g. const isMounted = useRef(); should work just as well.
There was a problem hiding this comment.
Nitpick aside, this looks and works fine to me!
I'm proposing to merge it, and proceed by iterating on it in smaller standalone PRs.
Working in feature branches will only make this much more complicated to review eventually.
We might also be more clear that this is a really experimental component.
I'm not sure if there's an established approach for this, but maybe we could add a note in the readme, or something like that. 🤔
* 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
|
A quick note on the "sliding sidebars" pattern. This pattern is not new in WordPress and it's, more or less, like the Customizer sidebars. Those are known to be an accessibility anti-pattern and are a terrible experience for keyboard users and assistive technologies users. When a new sidebar "slides in", the previous content disappears producing a loss of context that's hard to understand for users, especially screen readers users. This was also quickly discussed during the latest weekly accessibility meeting and probably the team will need to discuss it again bt I'm pretty sure the team has big concerns on the usage of this pattern, being also, as said, a known a11y issue. |
@afercia Thanks for chiming in! When the user navigates from a level to another, the former level un-renders and the latter renders. Would it make sense if on level change we automatically focused the first item of the new level? Should we instead try to work with always-rendered nested lists, and only toggle their visibility (which is kinda complicated, but that's not the point here 🙂)? |
|
@Copons thanks for the ping.
I'm not sure that makes a difference. Whether a part of a UI that is currently used by users gets hidden or removed from the DOM, that's a big accessibility problem. There's a complete loss of context. Information on the number of items in the list and their nesting level is broken. Screen reader users wouldn't have a clue that a part of the UI just doesn't exist any longer (why should they?). Moving focus programmatically should be rare and only implemented when it's the expected behavior for some ARIA pattern. In my years of experience in contributing to the accessibility team I can say with no doubt that the Customizer sidebar is one of the patterns we received a lot of complaints about. Also, I would like to note that in other parallel issues about sidebars, there are ongoing explorations and discussions on how to move away from the concept of sidebars. |
|
Closing as replaced by #25057. Thank you so much @psealock and @joshuatf for working on this! I've also especially appreciated how gracefully y'all reacted to me casually showing up out of the blue and trampling all over the place. 🙇♂️ |
Description
Expose a
<Navigation />component from@wordpress/componentsto render an array of menu items in a hierarchal fashion. The component is highly composable, accepting a child function that allows full control to the consuming application. It is also free from excessive styling so it can be adapted to many UIs.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: This PR introduces a new component.
Checklist: