Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Try] Navigation Component: Internal groupings #25035

Closed
wants to merge 2 commits into from
Closed

[Try] Navigation Component: Internal groupings #25035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8197cd4
try internal sections
psealock Sep 2, 2020
a23b502
First pass working
psealock Sep 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions packages/components/src/navigation/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,42 @@ const Navigation = ( { activeItemId, children, data, rootTitle } ) => {
};

const mapItems = ( itemData ) => {
const groupings = [];
const items = new Map(
[
{ id: 'root', parent: null, title: rootTitle },
...itemData,
].map( ( item ) => [ item.id, appendItemData( item ) ] )
[ { id: 'root', parent: null, title: rootTitle }, ...itemData ]
.filter( ( item ) => {
if ( item.type === 'grouping' ) {
item.children = [];
groupings.push( item );
return false;
}
return true;
} )
.map( ( item ) => [ item.id, appendItemData( item ) ] )
);

items.forEach( ( item ) => {
const parentItem = items.get( item.parent );
if ( parentItem ) {
parentItem.children.push( item );
parentItem.hasChildren = true;
if ( item.group ) {
const grouping = groupings.find(
( group ) => group.id === item.group
);
if ( grouping ) {
grouping.children.push( item );
}
} else {
parentItem.children.push( item );
parentItem.hasChildren = true;
}
}
} );

groupings.forEach( ( grouping ) => {
const parentItem = items.get( grouping.parent );
if ( parentItem ) {
parentItem.groupings = parentItem.groupings || [];
parentItem.groupings.push( grouping );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Copons Copons Sep 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go this route though, it's probably a refactor that should happen separately and not block the current PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

is this something that we can provide using hooks and context. an API like that could be awesome:

const [ screenId, params ] = useActiveScreen();

Copy link
Contributor

@Copons Copons Sep 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

}
} );

Expand Down
55 changes: 39 additions & 16 deletions packages/components/src/navigation/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,23 @@ const data = [
id: 'child-2',
parent: 'item-3',
},
{
title: 'Secondary Menu',
id: 'grouping-1',
parent: 'item-3',
type: 'grouping',
},
{
title: 'Nested Category',
id: 'child-3',
parent: 'item-3',
group: 'grouping-1',
},
{
title: 'Child 4',
id: 'child-4',
parent: 'item-3',
group: 'grouping-1',
},
{
title: 'Sub Child 1',
Expand Down Expand Up @@ -82,6 +95,16 @@ const data = [
function Example() {
const [ active, setActive ] = useState( 'item-1' );

const renderItem = ( item ) => (
<NavigationMenuItem
{ ...item }
key={ item.id }
onClick={
! item.href ? ( selected ) => setActive( selected.id ) : null
}
/>
);

return (
<>
{ active !== 'child-2' ? (
Expand All @@ -104,22 +127,22 @@ function Example() {
) }
<h1>{ level.title }</h1>
<NavigationMenu>
{ level.children.map( ( item ) => {
return (
<NavigationMenuItem
{ ...item }
key={ item.id }
onClick={
! item.href
? ( selected ) =>
setActive(
selected.id
)
: null
}
/>
);
} ) }
{ level.children.map( renderItem ) }
{ level.groupings
? level.groupings.map( ( grouping ) => (
<>
<h2>{ grouping.title }</h2>
<NavigationMenu
key={ grouping.id }
title={ grouping.title }
>
{ grouping.children.map(
renderItem
) }
</NavigationMenu>
</>
) )
: null }
</NavigationMenu>
</>
);
Expand Down