Make nav components more atomic #24266

joshuatf · 2020-07-29T09:09:01Z

Description

Trac ticket: https://core.trac.wordpress.org/ticket/47012

Breaks down nav components into smaller pieces so that the nav itself is more customizable.

This is in need of testing and there are still some outstanding issues with accessibility, but prefer to handle these in follow-ups to keep the scope of this PR down.

This PR tries to strike a balance between flexibility and ease of use. I think in keeping this component as presentational as possible, it would be ideal to have the consumer map over and use navigational components directly:

<NavigationMenu>
  <NavigationMenuItem
    onClick={ ... }
  >
     Menu Item
  </NavigationMenuItem>
<NavigationMenu />

instead of passing an array of items:

<Navigation active={ active } items={ items }>
  <NavigationMenu />
</Navigation>

However, using the former option means putting more responsibility on the consumer to filter the array of items. Open to feedback here and whether or not there's a way to improve this.

Screenshots

Testing

Run npm run storybook:dev
Check the Navigation component.
Check that items are rendered, navigation, and back buttons work, and the navigation title shows the correct title.

joshuatf · 2020-07-29T12:38:37Z

packages/components/src/navigation/stories/index.js

+	} );
+
+	return (
+		<Navigation active={ active } items={ items }>


Instead of passing an array of items here I'd prefer to have the consumer iterate over the items and create navigation items:

{ items.map( item => <NavigationMenuItem>{ item.title }</NavigtionMenuItem> }

However, this means the top level Navigation component does not have access to the full array and can't parse information needed for the title and back button components.

However, this means the top level Navigation component does not have access to the full array and can't parse information needed for the title and back button components.

One way to get around this can be to have <Navigation /> accept a single child that is a function. This way, any parameters can be supplied as function arguments. Something along these lines?

<Navigation active={ active } items={ data }> { ( { title, parent } ) => ( <> <NavigationBackButton parent={ parent } /> <NavigationTitle title={ title } /> <NavigationMenu> { data.map( ( item ) => ( <NavigationMenuItem key={ item.id } item={ item } isActive={ item.id === active } /> ) ) } </NavigationMenu> </> ) } </Navigation>

See the Tab Panel as an example:
https://github.com/WordPress/gutenberg/tree/master/packages/components/src/tab-panel

joshuatf · 2020-07-29T12:39:24Z

packages/components/src/navigation/stories/index.js

-		parent: 'root',
-		back: 'WooCommerce Home',
-		menu: 'primary',
+		id: 'analytics',


Updated to id to match terms used in the nav project, but open for discussion. My thinking is that slug may be confused with the slug used in the URL.

joshuatf · 2020-07-29T12:45:53Z

packages/components/src/navigation/menu.js

+			{ visibleItems.map( ( item ) => {
+				const children = getChildren( item );
+				return (
+					<NavigationMenuItem


I'm not sure that this will be very accessible and will probably require a follow-up. What I think we really need here is another component for NavigationSubmenu or NavigationMenuGroup to house and group submenus along with their titles. This may also make animation easier.

Are there accessibility issues with all the menus being side by side instead of nested? E.g., <ul></ul><ul></ul> vs <ul><ul></ul</ul> - The latter seems to be difficult to make work with the current waterfall design.

Introduced a NavigationPane component. I think in terms of animations, we'll need to render multiple panes simultaneously, which should be trivial given the new rendering approach.

Some aria attributes will also be needed to link these together since the uls will not be nested, but side by side.

psealock

Great start to this concept @joshuatf. I have some suggestions on the API and usage with a suggestion that could be useful in supplying information to the child elements without having to cloneElement. What do you think?

psealock · 2020-07-30T03:31:37Z

packages/components/src/navigation/stories/index.js

+				rootText="WooCommerce Home"
+				onClick={ ( item ) => ( item ? setActive( item.id ) : null ) }
+			/>
+			<NavigationTitle rootText="WooCommerce Home" />


The back button is always supposed to go up just one level, so in a multi depth config this will need to be dynamic.

I think the back button actually does more than go up one level. According to the demo, it also sets the first item from the higher level as the active page.

The way it's set up here passes the item (from the parent) that will be activated.

psealock · 2020-07-30T03:35:40Z

packages/components/src/navigation/stories/index.js

+	const items = data.map( ( item ) => {
+		item.onClick = () => setActive( item.id );
+		return item;
+	} );


This is an awkward API. I think it would be better to leave data as purely data. Instead of asking users to manipulate their data, can we just supply a prop for a callback?

This is not part of the API, this is part of the user supplied data. It may look odd because of our Example setup, but this is just supplying the onClick behavior as data:

[ { id: 'item-1'. onClick: () => setActive( 'item-1' ), } ]

psealock · 2020-07-30T03:47:52Z

packages/components/src/navigation/stories/index.js

+	} );
+
+	return (
+		<Navigation active={ active } items={ items }>


However, this means the top level Navigation component does not have access to the full array and can't parse information needed for the title and back button components.

One way to get around this can be to have <Navigation /> accept a single child that is a function. This way, any parameters can be supplied as function arguments. Something along these lines?

<Navigation active={ active } items={ data }> { ( { title, parent } ) => ( <> <NavigationBackButton parent={ parent } /> <NavigationTitle title={ title } /> <NavigationMenu> { data.map( ( item ) => ( <NavigationMenuItem key={ item.id } item={ item } isActive={ item.id === active } /> ) ) } </NavigationMenu> </> ) } </Navigation>

See the Tab Panel as an example:
https://github.com/WordPress/gutenberg/tree/master/packages/components/src/tab-panel

joshuatf · 2020-07-30T11:51:21Z

Thanks for all the feedback here, Paul! I wasn't very happy with my previous attempt at this, but I think we're getting closer.

One way to get around this can be to have accept a single child that is a function. This way, any parameters can be supplied as function arguments. Something along these lines?

I had considered this and went back and forth a few times, but I think you're right. This provides a much more flexible approach. This will also probably be necessary for the follow-up to address #24266 (comment). Updated in e7c0011

I have two different approaches for rendering nav items that can be seen in 53b85b2. I'm personally a much bigger fan of the previous approach (left-hand side of this diff) as this allows the consumer to be much more declarative and have a finer level of control over the menu items. It does come at the cost of being a bit more verbose though. Do you have any strong feelings one way or another on this?

psealock · 2020-07-30T18:57:22Z

packages/components/src/navigation/stories/index.js

+									? setActive( parentItems[ 0 ].id )
+									: ( window.location =
+											'https://wordpress.com' )
+							}


I think we can hide much of this logic. I don't think the consumer should worry about this much logic here and other places. I would prefer they just supply a data tree and call backs:

<NavigationBackButton parent={ parent } exitText="Dashboard" exitUrl="http://wordpress.com" />

But then maybe we should just incorporate this as part of the main parent component.

For animation purposes, I don't think we can have this as part of the main parent component. I think it would be possible to include it in the NavigationPane component, but I'm also concerned that our use case in WooCommerce is pretty specific and will not match the needs of consumers.

Do we think that most consumers will have exit buttons? That most will activate the parent's first child on click? If yes to both of those, then maybe we can include it, but this does feel very specific to our purposes.

For animation purposes, I don't think we can have this as part of the main parent component.

In the interest of this PR, maybe best to remove then. There are several ways to animate this and I think it'll best be evaluated in a separate PR.

Do we think that most consumers will have exit buttons?

Why don't we supply an onBack prop that takes a function with an id passed as the argument? I see what you're saying about it being a Woo specific use case at the top level.

That most will activate the parent's first child on click?

Yes, most definitely.

Why don't we supply an onBack prop that takes a function with an id passed as the argument?

The problem with this is that we're assuming the callback only needs an ID, which works well for our example that only works on IDs, but it may also need access to the onClick or url params of the first item.

@psealock One compromise here may be to pass the parent's first child by default as backItem if we're sure this is the item most consumers will want to navigate to. This way it can still pass all the details and we can treat those like props in this component so it can define url or onClick behavior.

I think there may also be some other ways to simplify this by using context and passing information by default to child components, but prefer to handle in a follow-up.

What do you think?

psealock · 2020-07-30T19:00:03Z

packages/components/src/navigation/pane.js

@@ -0,0 +1,5 @@
+const NavigationPane = ( { children } ) => {
+	return <div className="components-navigation__pane">{ children }</div>;
+};


Why not just render the div as part of the output of <Navigation />?

This may be a little pre-emptive, but there will be more logic around show multiple panes at once and its visibility for animation. See #24266 (comment)

psealock · 2020-07-30T19:15:30Z

I have two different approaches for rendering nav items that can be seen in 53b85b2. I'm personally a much bigger fan of the previous approach (left-hand side of this diff) as this allows the consumer to be much more declarative and have a finer level of control over the menu items. It does come at the cost of being a bit more verbose though. Do you have any strong feelings one way or another on this?

I prefer the less verbose way. I don't think consuming app needs to know about too much implementation. The important parts they need to control are onSelect and what is rendered for each item. I don't think handing off things like, for instance, the text suppled to the back button { ! parentLevel ? 'Dashboard' : parentLevel.title } is allowing them any advantage.

joshuatf · 2020-07-30T19:53:27Z

I prefer the less verbose way. I don't think consuming app needs to know about too much implementation. The important parts they need to control are onSelect and what is rendered for each item. I don't think handing off things like, for instance, the text suppled to the back button { ! parentLevel ? 'Dashboard' : parentLevel.title } is allowing them any advantage.

As noted above, the reasons for doing this are really to make a consumable component that also fits the use case we need for WooCommerce. Some of these requirements, like having a back button when there is nowhere to go back to ( { ! parentLevel ? 'Dashboard' : parentLevel.title }, feel very specific to our use case.

I agree on keeping the internal logic away from consumers as much as possible, but I do think the verbose alternative is mostly presentational. The pieces that are more logical are those that feel out of place in the component and specific to the WooCommerce nav. But if you feel that the same behavior would be consistent across all other consumers across nav components, then I'll internalize the logic further.

psealock · 2020-08-04T22:21:03Z

I agree on keeping the internal logic away from consumers as much as possible, but I do think the verbose alternative is mostly presentational.

I see what you're saying here and did some tinkering and came to some conclusions:

If the info is purely presentational, we should eliminate both NavigationTitle and NavigationBackButton from the component. We're not saving anyone time by just wrapping a regular Button around some text. If were not including styles as part of this component and seek to offer the highest degree of flexibility, I see the inclusion of NavigationTitle and NavigationBackButton as distractions in the development experience.

Taking this step a bit further, I know we really wanted to be able to designate a secondary menu like <NavigationMenu menu"secondary" />, but as it is now, we're filtering data at the prop level, ie:

items={ currentItems.filter(
    ( item ) => item.menu === 'secondary'
) }

This is definitely a Woo abstraction. Lets eliminate this from the example in Storybook too. In other words, we can make this component just handle the waterfall and animation aspects. The way you have it set up allows devs to insert a button and title any way they'd like.

I really like the way you have set up the data so that devs only need to supply title, id, and parent. Its really clean and I tried multiple levels of depth and it works well.

I still think we can remove NavigationPane. That may be the path forward, but I'd rather that be in a follow up PR.

joshuatf · 2020-08-05T10:36:13Z

If the info is purely presentational, we should eliminate both NavigationTitle and NavigationBackButton from the component.

Agreed, that makes sense to me. Removed.

This is definitely a Woo abstraction. Lets eliminate this from the example in Storybook too.

Simplified the example a good bit to the bare minimum to showcase features. I think it's a bit easier to follow now.

I still think we can remove NavigationPane. That may be the path forward, but I'd rather that be in a follow up PR.

Sure, happy to take it out for the time being.

Thanks for the feedback, @psealock. This is ready for another review.

One potential issue I realized while working on tihs- what if a nav has only category items for its top level (e.g., Cat1, Cat2, and Cat3). What happens if you navigate to "Cat1 - Child?" The back button typically activates the parent menus first item, which in this case would be "Cat1" and is not an actual page. /cc @jameskoster for your thoughts on this.

jameskoster · 2020-08-05T15:30:42Z

I spoke about that interaction with some of the other designers last week. Some consensus formed around the notion of clicking parent items (or the back button) not navigating to another screen, as a flat rule. There are trade-offs to this, but it felt like a good place to start. Prototype here to demonstrate. Sorry to flip-flop on this, I know this is the opposite of what we originally wanted to do for Woo 😬

cc @shaunandrews for any further insight.

psealock · 2020-08-05T21:40:06Z

Prototype here to demonstrate. Sorry to flip-flop on this, I know this is the opposite of what we originally wanted to do for Woo 😬

Originally, I thought one way to get around this was require at least one static page (ie, a nav item with no children) at each level. Now that I played with the prototype, I really like the independent nature of the menu. You can discover or search other parts of the hierarchy without having to move from the current page.

psealock

Ok nice @joshuatf, I feel like we are getting somewhere!

The interface is feeling cleaner with the introduction of NavigationMenuItem and removing the Woo abstractions makes it easier to focus on the right aspects.

How do you feel about naming things differently? Here is my take:

currentLevel - This is like the parent item whose children are on display. How about navMenu ?
currentItems - Children items being displayed. How about navItems?
parentItems - These are the aunts and uncles of the displayed children. How about ommers? There isn't really a good name for this, but I found ommers here. I think we should keep this internal, at least for now.
parentLevel - This would be the grandparent of the displayed children. I think this can just be grandparent and we can keep it internal as well.
backItem - This is the first aunt or uncle (or ommer), but depending on the new interaction Jay mentions above, it could be something else. How about navBack?

Separately, it feels like a lot of looping in the main Navigation component with O(n^2). We could explore simplifying in a follow up.

And finally, we can also iterate on the new "Back" interaction in a follow up. I'm ok with keeping the logic already in place for now.

Awesome work Josh!

psealock · 2020-08-05T22:51:35Z

packages/components/src/navigation/index.js

-		if ( parentalSiblings.length ) {
-			setActive( parentalSiblings[ 0 ] );
-		}
+const Navigation = ( { activeId, children, items: rawItems, rootTitle } ) => {


How about calling the items prop data?

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

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.

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

psealock · 2020-08-05T23:23:47Z

packages/components/src/navigation/menu.js

@@ -0,0 +1,5 @@
+const NavigationMenu = ( { children } ) => {
+	return <ul className="components-navigation__menu">{ children }</ul>;
+};


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.

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.

Right, good point.

joshuatf · 2020-08-10T15:36:04Z

I agree with your sentiments on naming, @psealock - these could use some work.

currentLevel - This is like the parent item whose children are on display. How about navMenu ?

I wanted something like level or category or pane to distinguish this. navMenu could be confused with the overall navigation menu instead of the current "category" we're in.

currentItems - Children items being displayed. How about navItems?

Same as above point. Maybe something like levelItems or paneItems or whatever we call the current level we're on.

parentItems - These are the aunts and uncles of the displayed children. How about ommers? There isn't really a good name for this, but I found ommers here.

My concern is that "ommers" is not a term many will be familiar with and not very intuitive for most.

I think we should keep this internal, at least for now.

I think with the changes that @jameskoster mentioned, we won't need this anymore, so agreed on this point.

parentLevel - This would be the grandparent of the displayed children. I think this can just be grandparent and we can keep it internal as well.

Yes, it technically is the grandparent of the displayed children. I think part of the confusion here may be that the relationship you're referencing is to the current children. Considering we're in the render for a given pane/level, I think it's the parent of the current pane. If we were inside a child item, then I think grandparent would be more appropriate.

backItem - This is the first aunt or uncle (or ommer), but depending on the new interaction Jay mentions above, it could be something else. How about navBack?

I would consider the last word Back to be the thing we're passing, which to me doesn't sound like an object (noun) and instead could be confused with an action (function).

It seems like we may have to keep the current level in state given the new design changes though, so this may not be something we need to pass.

joshuatf · 2020-08-10T16:34:48Z

@psealock I made some updates to this in aca45b5.

This removes the backItem and parentItems. I kept the naming structure similar, but I think it accurately represents the current level we're in, using level (current pane or level we're on), levelItems, and parentLevel which is the parent to the current level.

Still open to renaming here, but I do feel the relationship semantics there are accurate.

This commit introduces a setActiveLevel method to update the level. I think in a future PR, we can clean this up further using context sharing.

psealock

I think setActiveLevel deserves its own PR and and discussion, but since its already here, I think its fine and we can refine it in the next one. When we work on this, I think it would be useful to add multiple levels of items and buttons that mimic a route change by calling setActive from the example so we can mock a real implementation.

I still don't feel great about level, but at least there is consistency and implied hierarchy now which is more intuitive.

I'm good to merge this and continue being open about naming. Just couple small comments and were good to merge and keep going.

psealock · 2020-08-11T04:07:02Z

packages/components/src/navigation/index.js

-import Text from '../text';
-import Item from './item';
+const Navigation = ( { activeId, children, items: rawItems, rootTitle } ) => {
+	const [ activeLevel, setActiveLevel ] = useState( 'root' );


The initial state of activeLevel is going to be the activeId.

activeId represents the item ID as opposed to the level ID. I renamed this activeItemId for the time being but we can rename later if that's still confusing.

psealock · 2020-08-11T04:12:54Z

packages/components/src/navigation/index.js

-				) }
-			</div>
+			{ children( {
+				activeId,


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

Good call. Removed.

joshuatf · 2020-08-11T12:03:56Z

Thanks for all the feedback and discussion on this one, @psealock!

I think setActiveLevel deserves its own PR and and discussion, but since its already here, I think its fine and we can refine it in the next one. When we work on this, I think it would be useful to add multiple levels of items and buttons that mimic a route change by calling setActive from the example so we can mock a real implementation.

Agreed 💯 . I would have preferred not to include it in this PR at all, but in order to get the concept working with these changes it seemed like the path of least resistance. I think a real-world routing example (and maybe even mix of various non-router links as well) will be very useful.

I still don't feel great about level, but at least there is consistency and implied hierarchy now which is more intuitive.

Very open to the naming on this. I like using the "level" as the reference to child/parents, but the name itself could probably use some work. I'm not really sure what this should be to make it more intuitive (pane/level/category/?).

* Break nav components into smaller pieces * Get visible menu items based on menu id and active state * Allow null and root text in back button * Allow root text for navigation title * Add line break to make use of multiple menus more obvious * Fix back button case at root level * Remove secondary nav styling * Simplify component props * Pass parsed data back to navigation children * Move nav menu item logic into menu * Simplify nav components to bare essentials * Allow menu level navigation without changing active item * Handle PR feedback

joshuatf requested review from ajitbohra, chrisvanpatten and youknowriad as code owners July 29, 2020 09:09

joshuatf added 7 commits July 29, 2020 15:31

Break nav components into smaller pieces

43c18f2

Get visible menu items based on menu id and active state

a4bbf76

Allow null and root text in back button

ee380c5

Allow root text for navigation title

f93fceb

Add line break to make use of multiple menus more obvious

9b7bc2f

Fix back button case at root level

937c3c7

Remove secondary nav styling

3444b29

joshuatf force-pushed the update/separate-components branch from d602b75 to 3444b29 Compare July 29, 2020 12:31

joshuatf mentioned this pull request Jul 29, 2020

Make nav components more atomic psealock/gutenberg#2

Closed

joshuatf commented Jul 29, 2020

View reviewed changes

psealock added the [Feature] Navigation Component A navigational waterfall component for hierarchy of items. label Jul 29, 2020

psealock reviewed Jul 30, 2020

View reviewed changes

joshuatf added 3 commits July 30, 2020 14:34

Simplify component props

f93d0ad

Pass parsed data back to navigation children

e7c0011

Move nav menu item logic into menu

53b85b2

psealock reviewed Jul 30, 2020

View reviewed changes

Simplify nav components to bare essentials

7b68ca6

psealock reviewed Aug 5, 2020

View reviewed changes

Allow menu level navigation without changing active item

aca45b5

psealock approved these changes Aug 11, 2020

View reviewed changes

Handle PR feedback

a5c4416

psealock merged commit b67e808 into WordPress:feature/navigation Aug 11, 2020

psealock mentioned this pull request Aug 11, 2020

Navigation Component: Remove internal state #24255

Closed

4 tasks

psealock mentioned this pull request Aug 20, 2020

Navigation Component: Create individual components to break down responsibility #24257

Closed

Make nav components more atomic #24266

Make nav components more atomic #24266

Conversation

joshuatf commented Jul 29, 2020 • edited by psealock Loading

Description

Screenshots

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psealock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuatf commented Jul 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psealock commented Jul 30, 2020

joshuatf commented Jul 30, 2020

psealock commented Aug 4, 2020

joshuatf commented Aug 5, 2020

jameskoster commented Aug 5, 2020

psealock commented Aug 5, 2020

psealock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuatf commented Aug 10, 2020

joshuatf commented Aug 10, 2020

psealock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuatf commented Aug 11, 2020

joshuatf commented Jul 29, 2020 •

edited by psealock

Loading

psealock left a comment •

edited

Loading