-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
0e0bb0c
016bd69
c92fb80
60fa5f2
647f332
be4c224
ff2a169
7ff8479
7ce0d8a
0a3d717
56be028
645018d
c89e13a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. |
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 } ) => { | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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): 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for the clarification. I think that means every menu item will need a distinction, ie 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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; |
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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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: 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; |
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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of defining a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 />; | ||
}; |
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.