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

Add Navigation component [Feature branch] #24107

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions docs/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -1043,6 +1043,12 @@
"markdown_source": "../packages/components/src/navigable-container/README.md",
"parent": "components"
},
{
"title": "Navigation",
"slug": "navigation",
"markdown_source": "../packages/components/src/navigation/README.md",
"parent": "components"
},
{
"title": "Notice",
"slug": "notice",
Expand Down
43 changes: 43 additions & 0 deletions packages/components/src/navigation/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Navigation

Navigation is a component which renders a component which renders a heirarchy of menu items.

## Usage

```jsx
import { MenuItem } from '@wordpress/components';
import { withState } from '@wordpress/compose';

const data = [
{ title: 'My Navigation', slug: 'root', back: 'Back' },
{ title: 'Home', slug: 'home', parent: 'root', menu: 'primary' },
{ title: 'Option one', slug: 'option_one', parent: 'root', menu: 'primary' },
{ title: 'Option two', slug: 'option_two', parent: 'root', menu: 'primary' },
{ title: 'Option three', slug: 'option_three', parent: 'root', menu: 'secondary' },
{ title: 'Child one', slug: 'child_one', parent: 'option_three', menu: 'primary' },
{ title: 'Child two', slug: 'child_two', parent: 'option_three', menu: 'primary' },
{ title: 'Child three', slug: 'child_three', parent: 'option_three', menu: 'primary' },
];

const MyNavigation = () => {
return <Navigation data={ data } initial="home" />;
};
```

## Props

Navigation supports the following props.

### `data`

- Type: `array`
- Required: Yes

An array of config objects for each menu item.

### `initial`

- Type: `string`
- Required: Yes

The active slug.
76 changes: 76 additions & 0 deletions packages/components/src/navigation/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/**
* 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 } ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is initial the active item? Might be worth renaming for clarity. Some prop types would be a great addition too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats my intention, but agree there is probably a better name. My thinking is that the active menu item will be passed down as a prop from either a router or any other outside logic. As I have it now, this component has state, which we'll need to figure out first (I see you started that discussion below) and hopefully naming of this prop will be more evident.

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 ] );
}
};

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
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to handle infinite levels of depth here, right? We may look at something similar to what I did here:

https://github.com/woocommerce/navigation/blob/9aa6ef1a2f9240d680521316277708d9021c6c17/client/menu-item/index.js#L35

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 question. From the prototype, it appears the primary v. secondary dichotomy only exists at the top level. Can you confirm this @jameskoster ? In other words, once you being drilling down, it will a become a single menu and there won't be a secondary section.

I added a multiple depth level to illustrate multiple levels of depth:

depth

Copy link
Contributor

Choose a reason for hiding this comment

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

In other words, once you being drilling down, it will a become a single menu and there won't be a secondary section.

That was the case for WooCommerce. I cannot say that it will also be the case for all other plugins, or core for that matter. Looking at the latest designs for core there seem to be separate sections within a drilldown (note 'recent' and 'drafts' sections):

sections

I'm not sure if they qualify as primary / secondary though... And these designs are still being worked on, so this should probably be taken with a grain of salt :D

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 cannot say that it will also be the case for all other plugins

Ok, thanks for the clarification. I think that means every menu item will need a distinction, ie menu: 'primary|secondary' as suggested by @joshuatf. Probably defaulting to primary.

That is of course unless we want to allow unlimited groupings 🤔 . For now, assuming a maximum of two sections will work and we can revamp later.

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 forward with @ItsJonQ's suggestion then I think we will end up removing primary and secondary logic altogether and this can be determined by the navigation plugin consuming this component.

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>
</div>
);
};

export default Navigation;
36 changes: 36 additions & 0 deletions packages/components/src/navigation/item.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* 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 Item = ( { data, item, setActive, isActive } ) => {
const children = data.filter( ( d ) => d.parent === item.slug );
const onSelect = () => {
const next = children.length ? children[ 0 ] : item;
setActive( next );
Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to get your thoughts on how the URL or callback would work here. Part of me is thinking that the whole nav component should remove stateless and we should pass the active item slug to the nav component and click handlers to the individual items to keep things unopinionated.

Since a plugin making use of this component may depend on React Router or may want to redirect to another page entirely, it may make sense just to let the items define what happens on click and allow the consuming plugin to define which item is active.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Part of me is thinking that the whole nav component should remove stateless

I feel this way too. I'd like to strive to have a "dumb" component as much as possible. I added state here because I was thinking of a scenario where there are multiple depths of menus which require the user to drill down and the page may not change until the user gets to the bottom. In this case the component would need to keep track of where it is in the tree.

But now I think this may be an edge case we can ignore for now. One solution would be to require at least one "page" for every level. For instance, the "Home" link below:

Screen Shot 2020-07-23 at 2 12 39 PM

I will remove state and simplify this component next.

};
const classes = classnames( 'components-navigation__menu-item', {
'is-active': isActive,
} );
return (
<Button className={ classes } onClick={ onSelect } key={ item.slug }>
<Text variant="body.small">
<span>{ item.title }</span>
</Text>
{ children.length ? <Icon icon={ chevronRight } /> : null }
</Button>
);
};

export default Item;
90 changes: 90 additions & 0 deletions packages/components/src/navigation/stories/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/**
* Internal dependencies
*/
import Navigation from '../';

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',
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of defining a back property for each of these, I would much rather we find the parent and display its title instead. Maybe we have a prop like rootTitle for defining what's displayed for the top level back button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is actually what is happening. The back button's text is populated by the parent menu item, eg parent.back. I do agree back is a poor name for the property. backButtonText ?

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: 'All orders',
slug: 'all_orders',
parent: 'orders',
},
{
title: 'Payouts',
slug: 'payouts',
parent: 'orders',
},
{
title: 'Settings',
slug: 'settings',
parent: 'root',
back: 'WooCommerce Home',
menu: 'secondary',
},
{
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',
},
];

function Example() {
return <Navigation data={ data } initial="home" />;
}

export const _default = () => {
return <Example />;
};
30 changes: 30 additions & 0 deletions packages/components/src/navigation/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
.components-navigation {
width: 272px;
padding: 35px 30px;
}

.components-navigation__back {
margin-bottom: 64px;
}

.components-navigation__title {
margin-bottom: 40px;
}

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

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

.components-navigation__menu-item {
display: flex;
justify-content: space-between;

&.is-active span {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add a hover state for the font color change or do you think this is something we should override in the nav repo?

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 think all buttons will need their own color overrides to the default Button component in this repo. I think this can be a follow up.

border-bottom: 2px solid var(--wp-admin-theme-color);
}
}
1 change: 1 addition & 0 deletions packages/components/src/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
@import "./menu-item/style.scss";
@import "./menu-items-choice/style.scss";
@import "./modal/style.scss";
@import "./navigation/style.scss";
@import "./notice/style.scss";
@import "./panel/style.scss";
@import "./placeholder/style.scss";
Expand Down