[Try] Navigation Component: Internal groupings #25035
Conversation
psealock
requested review from
ajitbohra,
chrisvanpatten and
youknowriad
as code owners
psealock
added
[Feature] Navigation Component
|
Size Change: +81 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
| const parentItem = items.get( grouping.parent ); | ||
| if ( parentItem ) { | ||
| parentItem.groupings = parentItem.groupings || []; | ||
| parentItem.groupings.push( grouping ); |
There was a problem hiding this comment.
I have a feeling that this is becoming a bit too magic, and had some problems following it (I'm not use to mutating stuff anymore 😅).
If I'm not mistaken, there is a lot of duplicated data in the resulting items Map, with data listed both nested and flat.
I'm poking around with this function, trying to come up with a reasonably simpler version.
It's not simple, I'll give you that! 😄
There was a problem hiding this comment.
This is will just become more complex over time, ideally we should move a way from "config" based API and use a component composition based one.
We have the same issue with DropdownMenu component where it's config based like the navigation one and unfortunately, we hit the limit on several occasions everytime we have a slightly different use-case.
There was a problem hiding this comment.
@youknowriad I've seen that DropdownMenu has 2 different interfaces, with config or children.
Building upon the children one, we could do something like this for the Navigation:
<Navigation>
{ ( { navigateTo, currentLevel, activeItem } ) => ( <Fragment>
<NavigationLevel
level="root"
title={ __( 'Home' ) }
isCurrentLevel={ 'root' === currentLevel }
>
<NavigationItem
slug="item-1"
title={ __( 'Item 1' ) }
badge={ 0 }
onClick={ navigateTo( 'sub-menu' ) }
isActive={ 'item-1' === activeItem }
/>
<NavigationItem
slug="external-link"
title={ __( 'External Link' ) }
href="#"
isActive={ 'external-link' === activeItem }
/>
</NavigationLevel>
<NavigationLevel
level="sub-menu"
title={ __( 'Sub Menu' ) }
isCurrentLevel={ 'sub-menu' === currentLevel }
>
<NavigationBackButton onClick={ navigateTo( 'root' ) } />
<NavigationItem
slug="child-1"
title={ __( 'Child 1' ) }
isActive={ 'child-1' === activeItem }
/>
</NavigationLevel>
</Fragment> ) }
</Navigation>With the navigateTo function controlling the whole toggle state and animation when traversing the menu hierarchy, and the various NavigationItem that could be either simple buttons or render anything inside.
Although I think this would be very hard to filter. 🤔
There was a problem hiding this comment.
Yes something like that.
I actually think the current config implementation could be built on top of a more flexible API based on components composition. So we'd have one API and potentially a shortcut (not sure if needed on Core though).
I think @ItsJonQ had some similar ideas on explorations maybe on his G2 project?
There was a problem hiding this comment.
If we go this route though, it's probably a refactor that should happen separately and not block the current PR.
There was a problem hiding this comment.
Do you think such refactor can happen after merging the main Navigation PR?
Since the current implementation has reached some stability, a large refactor won't cause conflicts with ongoing work. We've been using npm link to the feature/navigation branch, so I'm ok to do a refactor before the merge.
we could do something like this for the Navigation:
The challenge that jumps out to me is that not every navigation view will have sub menus and some will have multiple submenus. How would you compose a render function to handle all those possibilities? I will have a ponder on that today and take inspiration from @ItsJonQ 's code to see if I can find a solution.
Thanks for the feedback @Copons @youknowriad !
There was a problem hiding this comment.
I've opened a (very rough!) draft of the approach I outlined earlier: #25057
Apologize for its state, but I wanted to push out something to avoid losing a day (a whole weekend even?) due to timezone differences. 🙂
There was a problem hiding this comment.
Thanks for that draft @Copons! Overall I do like the compositional approach more.
However, we do shift most of the responsibility for determining active items and active levels to the consumer. If we're okay with this then I'm all for the refactor.
There was a problem hiding this comment.
However, we do shift most of the responsibility for determining active items and active levels to the consumer. If we're okay with this then I'm all for the refactor.
is this something that we can provide using hooks and context. an API like that could be awesome:
const [ screenId, params ] = useActiveScreen();
There was a problem hiding this comment.
@youknowriad the current version of the composition approach #25057 does indeed use the Context API.
I haven't moved the "active" logic into a custom hook as it's very simple and straightforward, but nothing blocks us to do so.
At the moment, the consumers don't need to know or do anything about the internal state of the navigation (although they optionally can).
|
Closing in favor of #25057 |
Fixes #24992
cc @joshuatf @Copons. This is rough try at groupings and would need a fair amount of cleanup. I'm open to suggestions and am not married to any of the naming. Thanks!
Description
The idea behind this branch is to allow the consumer to supply a data object for a grouping that looks like this:
The data is then prepared such that a
levelcan not only havechildrenbutgroupingsas well.groupingswould be an array of groups, each with their own children.The consumer renders the child items as well as each grouping's child items. The result is an ability to render groups at any portion of the waterfall without having to know anything about the details of the group
How has this been tested?
npm run storybook:dev