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.

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

Make nav components more atomic #24266

Merged
108 changes: 41 additions & 67 deletions packages/components/src/navigation/index.js
Original file line number Diff line number Diff line change
@@ -1,74 +1,48 @@
/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';
import { Icon, arrowLeft } from '@wordpress/icons';

/**
* Internal dependencies
*/
import Button from '../button';
import Text from '../text';
import Item from './item';

const Navigation = ( { data, initial } ) => {
const initialActive = data.find( ( item ) => item.slug === initial );
const [ active, setActive ] = useState( initialActive );
const parent = data.find( ( item ) => item.slug === active.parent );
const items = data.filter( ( item ) => item.parent === active.parent );

const goBack = () => {
if ( ! parent.parent ) {
// We are at top level, will need to handle this case.
return;
}
const parentalSiblings = data.filter(
( item ) => item.parent === parent.parent
);
if ( parentalSiblings.length ) {
setActive( parentalSiblings[ 0 ] );
}
const Navigation = ( { activeId, children, items: rawItems, rootTitle } ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about calling the items prop data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

data is much more generic than items. Do you consider the given data to not just be "items?"

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you consider the given data to not just be "items?"

No, this is why you have to rename them rawItems. Maybe rawItems should be data if you are firm on keeping the external props items.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see your point. Updated this to data.

const mapItemData = ( items ) => {
return items.map( ( item ) => {
const itemChildren = rawItems.filter(
( i ) => i.parent === item.id
);
return {
...item,
children: itemChildren,
parent: item.parent || 'root',
isActive: item.id === activeId,
hasChildren: itemChildren.length > 0,
};
} );
};
const items = [
{ id: 'root', title: rootTitle },
...mapItemData( rawItems ),
];

const activeItem = items.find( ( item ) => item.id === activeId );
const currentLevel = activeItem
? items.find( ( item ) => item.id === activeItem.parent )
: { id: 'root', title: rootTitle };
const parentLevel = currentLevel
? items.find( ( item ) => item.id === currentLevel.parent )
: null;
const currentItems = currentLevel
? items.filter( ( item ) => item.parent === currentLevel.id )
: items.filter( ( item ) => item.parent === 'root' );
const parentItems = parentLevel
? items.filter( ( item ) => item.parent === parentLevel.id )
: [];
const backItem = parentItems.length ? parentItems[ 0 ] : null;

return (
<div className="components-navigation">
<Button
isSecondary
className="components-navigation__back"
onClick={ goBack }
>
<Icon icon={ arrowLeft } />
{ parent.back }
</Button>
<div className="components-navigation__title">
<Text variant="title.medium">{ parent.title }</Text>
</div>
<div className="components-navigation__menu-items">
{ items.map( ( item ) =>
item.menu !== 'secondary' ? (
<Item
key={ item.slug }
data={ data }
item={ item }
setActive={ setActive }
isActive={ item.slug === active.slug }
/>
) : null
) }
</div>
<div className="components-navigation__menu-items is-secondary">
{ items.map( ( item ) =>
item.menu === 'secondary' ? (
<Item
key={ item.slug }
data={ data }
item={ item }
setActive={ setActive }
isActive={ item.slug === active.slug }
/>
) : null
) }
</div>
{ children( {
activeId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets omit activeId. Its the same as the supplied activeId prop anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Removed.

backItem,
currentItems,
currentLevel,
parentItems,
parentLevel,
} ) }
</div>
);
};
Expand Down
36 changes: 0 additions & 36 deletions packages/components/src/navigation/item.js

This file was deleted.

39 changes: 39 additions & 0 deletions packages/components/src/navigation/menu-item.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { Icon, chevronRight } from '@wordpress/icons';

/**
* Internal dependencies
*/
import Button from '../button';
import Text from '../text';

const NavigationMenuItem = ( props ) => {
const { children, hasChildren, isActive, onClick, title } = props;
const classes = classnames( 'components-navigation__menu-item', {
'is-active': isActive,
} );

const handleClick = () => {
onClick( children.length ? children[ 0 ] : props );
};

return (
<li className={ classes }>
<Button className={ classes } onClick={ handleClick }>
<Text variant="body.small">
<span>{ title }</span>
</Text>
{ hasChildren ? <Icon icon={ chevronRight } /> : null }
</Button>
</li>
);
};

export default NavigationMenuItem;
5 changes: 5 additions & 0 deletions packages/components/src/navigation/menu.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const NavigationMenu = ( { children } ) => {
return <ul className="components-navigation__menu">{ children }</ul>;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the <Navigation /> component absorb this ul? We can still have the separation of different sections by filtering the <NavigationMenuItem />'s according to primary or secondary in the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we can. If Navigation absorbs the ul then we will no longer be able to include other non-li elements under here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, good point.


export default NavigationMenu;
115 changes: 54 additions & 61 deletions packages/components/src/navigation/stories/index.js
Original file line number Diff line number Diff line change
@@ -1,88 +1,81 @@
/**
* WordPress dependencies
*/
import { Button } from '@wordpress/components';
import { useState } from '@wordpress/element';

/**
* Internal dependencies
*/
import Navigation from '../';
import NavigationMenu from '../menu';
import NavigationMenuItem from '../menu-item';

export default {
title: 'Components/Navigation',
component: Navigation,
};

const data = [
{ title: 'WooCommerce', slug: 'root', back: 'Dashboard' },
{ title: 'Home', slug: 'home', parent: 'root', menu: 'primary' },
{
title: 'Analytics',
slug: 'analytics',
parent: 'root',
back: 'WooCommerce Home',
menu: 'primary',
},
{
title: 'Orders',
slug: 'orders',
parent: 'root',
back: 'WooCommerce Home',
menu: 'primary',
},
{
title: 'Overview',
slug: 'overview',
parent: 'analytics',
},
{
title: 'Products report',
slug: 'products',
parent: 'analytics',
title: 'Item 1',
id: 'item-1',
},
{
title: 'All orders',
slug: 'all_orders',
parent: 'orders',
title: 'Item 2',
id: 'item-2',
},
{
title: 'Payouts',
slug: 'payouts',
parent: 'orders',
title: 'Category',
id: 'item-3',
},
{
title: 'Settings',
slug: 'settings',
parent: 'root',
back: 'WooCommerce Home',
menu: 'secondary',
title: 'Child 1',
id: 'child-1',
parent: 'item-3',
},
{
title: 'Extensions',
slug: 'extensions',
parent: 'root',
back: 'WooCommerce Home',
menu: 'secondary',
},
{
title: 'General',
slug: 'general',
parent: 'settings',
},
{
title: 'Tax',
slug: 'tax',
parent: 'settings',
},
{
title: 'My extensions',
slug: 'my_extensions',
parent: 'extensions',
},
{
title: 'Marketplace',
slug: 'marketplace',
parent: 'extensions',
title: 'Child 2',
id: 'child-2',
parent: 'item-3',
},
];

function Example() {
return <Navigation data={ data } initial="home" />;
const [ active, setActive ] = useState( 'item-1' );

return (
<Navigation activeId={ active } items={ data } rootTitle="Home">
{ ( { currentItems, currentLevel, backItem } ) => {
return (
<>
{ backItem && (
<Button
isPrimary
onClick={ () => setActive( backItem.id ) }
>
Back
</Button>
) }
<h1>{ currentLevel.title }</h1>
<NavigationMenu>
{ currentItems.map( ( item ) => {
return (
<NavigationMenuItem
{ ...item }
key={ item.id }
onClick={ ( selected ) =>
setActive( selected.id )
}
/>
);
} ) }
</NavigationMenu>
</>
);
} }
</Navigation>
);
}

export const _default = () => {
Expand Down
6 changes: 1 addition & 5 deletions packages/components/src/navigation/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,9 @@
margin-bottom: 40px;
}

.components-navigation__menu-items {
.components-navigation__menu {
display: flex;
flex-direction: column;

&.is-secondary {
margin-top: 24px;
}
}

.components-navigation__menu-item {
Expand Down