Add Navigation component [Feature branch] #24107

psealock · 2020-07-22T02:50:29Z

Add a page navigator component for use in the Block Editor and WooCommerce's Navigation project.

See a full demo here to see WooCommerce's application and check out a screenshot from Block Editor designs below:

Current state

This is a work in progress. To see the latest, use Storybook by running npm run storybook:dev.

How has this been tested?

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

joshuatf

Fantastic start, Paul!

Added some comments as well as a link to the component I was working on in the nav repo for reference.

Definitely want to get some tests around this as well, but I think we can tackle in a follow-up since this is already a lot to bite off in a single PR.

packages/components/src/navigation/item.js

joshuatf · 2020-07-22T08:18:14Z

packages/components/src/navigation/index.js

+import Text from '../text';
+import Item from './item';
+
+const Navigation = ( { data, initial } ) => {


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

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.

joshuatf · 2020-07-22T08:19:54Z

packages/components/src/navigation/style.scss

+}
+
+.components-navigation-title {
+	// For now


Will this change?

Yeah, I think there's probably a better way to layout this component than just margin-bottom to elements. Maybe I should remove as well.

joshuatf · 2020-07-22T08:24:38Z

packages/components/src/navigation/index.js

+			<div className="components-navigation-title">
+				<Text variant="title.medium">{ parent.title }</Text>
+			</div>
+			<div className="components-navigation-items">


Would love to see this broken down into Menu components to keep things dry.

Good abstraction to keep in mind once we have the data model and usage sorted out.

packages/components/src/navigation/index.js

joshuatf · 2020-07-22T08:51:11Z

packages/components/src/navigation/style.scss

+.components-navigation-item {
+	display: flex;
+	justify-content: space-between;
+	margin-left: -12px;


This feels a little duct-tapey to me. Maybe we could create a scss variable for padding that's used for all items instead of using negative margin.

Yes, agree here. I opted to use just enough CSS to make it look respectable. I'm hoping to concentrate more on the logic at this stage until the markup and componentry are finalised. Maybe its best to remove so it doesn't get left in there?

packages/components/src/navigation/style.scss

joshuatf · 2020-07-22T10:15:33Z

packages/components/src/navigation/style.scss

+	flex-direction: column;
+}
+
+.components-navigation-item {


Menu items with long text may be capped by height and I think we should left-align the text here. Default height in designs looks to be about 40px.

Do we also want to remove the focus outline on menu items here?

I think we'll need to keep focus outline for keyboard users. At least change the color or something to make it more presentable.

Good point on the long names, that will certainly be an issue.

packages/components/src/navigation/style.scss

joshuatf · 2020-07-22T10:17:18Z

packages/components/src/navigation/style.scss

+	margin-left: -12px;
+	color: #8f98a1;
+
+	&.is-active span {


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?

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.

ItsJonQ · 2020-07-22T17:15:36Z

Hiya all!

@psealock Oh Wow! Thank you for getting the ball rolling on this :D. I know this is still early in development! Hopefully there's still room for feedback.

I've been researching and experimenting very heavily in this space (Navigation components). I was planning on creating a draft PR with some ideas, but you beat me to it 😉 .

I'm going to share some of my thoughts and findings (I hope that's okay!).
Just a heads up, it's going to be long.

Identifying Requirements

I've studied various (nested) navigation use cases from various websites, apps, mobile apps, design systems, and component libraries. All with the goal of understanding how we could apply some of those patterns to a potential Navigation component for WordPress/Gutenberg.

I think I've boiled it down to handful of important requirements:

Supports multiple layers of nesting
Nav items should be "active" aware (e.g. renders bold on current item)
Clicking on nav items can affect things outside the nav menu
Outside changes can affect state of the nav menu

Other important implementation details include:

State changes should be "back"/"forward" aware (for animation purposes)
Should gracefully (preferably automatically) handle transition renders (e.g. mounting/unmounting)

Router?

The above behaviours are typically includes as part of a Router system. (e.g. React Router). Where we run into some issues is...

Gutenberg doesn't use a Router (at all).
Using a Router (e.g. React Router) for the a nav menu means that it will not be compatible with 3rd party apps using the same Router. (Router context cannot be gracefully shared, as the libraries are built to assume 1 single router instance).

In other words... We'd like to have the feature-set of a Router, without directly using a library (like react-router).

Inspiration

The closest example of something that ticks all of the boxes is react-navigation (the go-to React Native solution).
I've analyzed their docs quite extensively in the hopes of identifying good component API patterns.

React Navigation allows the dev to create many different navigations (if needed). More importantly, these navs can be mixed together. Jumping between can be seamlessly triggered.

Anatomy

We could break down the major parts of this Navigation menu into several parts:

Container - Single element that contains all of the Screen elements.
Screen - Element that contains content. Identified by a specific key.
Link - Clicking can navigate Screen elements. Is "active aware" (e.g. renders bold on current item)
navigate - A series of callback functions for finer control in navigating Screen elements.

(Note: Excuse the pseudo JSX code. I've naively simplified it for illustration purposes)

An example of a configuration may look like this:

const MyNav = () => {
    return (
        <Container>
            <Screen id="Home" component={Home} />
            <Screen id="Pages" component={Pages} />
            <Screen id="Posts" component={Posts} />
        </Container>
    )
}

Almost identical to react-router.

Container

The Container should hold all of the Screen elements. However, there should be logic that shows only a single Screen, matching current logic. For example, if the state is currently set as Home, it will show the Screen with the id of Home.

Transitioning

On a state change, the Container would coordinate the transitions and necessary mount/unmount lifecycles with the previous and next Screen. It should also be aware of "back" and "forward" navigations (just like a browser or how iOS/Android handles navigating screens). If animations are enabled, it will animate accordingly to directional flow.

Adapting to incoming changes

The <Container /> should be smart. It may need to transition Screen elements based on external factors.

For example, let's imagine the app setup looks something like this:

<App>
    <Sidebar>
        <Container>
            ...
        </Container>
    </Sidebar>
    <Main>
        ...
    </Main>
</App>

First, our Navigation container needs to know which Screen to render on initial load. This may come from parsing the URL directly. Ideally though, a component in a higher-level would pass some data along to <Container />. Something like...

<Container current={currentId}>
    ...
</Container>

Let's say... we click on something in the <Main /> area, causing content in the <Main /> area to shift around. However, we also need our navigation <Container /> to update.

If we are able to adjust the currentId in some manner (at the higher level), then we'll be able to successfully synchronize external actions to internal Navigation state.

Screen

The Screen should be able to render anything inside. If we follow the react-router like approach, it is achieved by accepting a component, like so:

<Screen id="Pages" component={Pages} />

Screen is responsible for coordinating with Container. And potentially passing down internal navigation information to the specified component. It may not be necessary as a hook may be able to accomplish this.

An example of an interesting (but still valid) use-case can be seen in some FSE navigation designs:
#23939

If we continue our Pages example...

The Pages component may need to do some asycn data fetching. Like getting a list of recent pages.

Since the component isn't tied directly to Screen, it is free to do so using it's own lifecycle methods (like useEffect hooks) and internal state.

Link / navigate

This is one of the trickier bits, as we may need to...

navigate the nav menu one it's own
trigger an action that change something externally (without affecting the nav menu)
or both!

The simplest way I can think of to accomplishing this it to perhaps rely on a series of navigate functions.

Here's an example:

// screens/Settings.jsx

const Settings = () => {
    const navigate = useNavigate()
    const { checkSomething } = useDispatch(...)

    return (
        <>
            <header>
                <button onClick={navigate.goBack}>Back</button>
                <h1>Settings</h1>
            </header>
            <ul>
                <li>
                    <Link onClick={() => navigate.to('Settings/Advanced')}>
                        Advanced
                    </Link>
                </li>
                 <li>
                    <Link onClick={checkSomething}>
                        Load More
                    </Link>
                </li>
            </ul>
        </>
    )
}

The navigate function to provides us with a flexible but light-weight way to coordinate custom use-cases.

React Navigate's doc about navigating describes it quite well!
https://reactnavigation.org/docs/navigating

Aesthetics

Feel like these navigation components should output unstyled HTML. The reason for this is because how Gutenberg/WP may need it to look, may different for 3rd party extensions like Woo (if Woo and others decide the use the same components). It would be difficult for 3rd party to adopt this and to have them fight with existing styles.

We can see this hands-off approach taken by libraries like Reakit to great success!

I feel like this navigation component may be able to achieve similar results, especially for the <Link /> component.

Example:

<Link to="Settings" as={MyCustomLink} className="my-custom-className">Settings</Link>

breathes

Oh boy! That was a long one. Thank you all your time and consideration 🙇‍♂️.

This is very tricky (but exciting) component! I'm happy that I'm able to participate.

psealock · 2020-07-23T10:00:48Z

@ItsJonQ Thats great feedback, you've done lots of thinking on the subject!

What you are describing is a fully controlled component that handles many aspects of navigation from routing, linking, and presenting content. The nature of the problem, as I see it, is one of placing a sidebar component in existing environments complete with their own routing solutions (or not), page controllers and/or presentational logic.

I think the path forward is creating a "dumb" component whose inputs are a data tree, a current screen id, and click handlers (or urls). If we remove the requirement for an internal state or router or any navigational logic, we can concentrate on a component that is solely responsible for presenting the appropriate tree, animating between screens, updating solely based on outside changes in screen id, and url navigation or executing a callback handed down as a prop.

The architectural ideas described can exist and be built around such a component. By removing the logic from this component in favor of a more presentational component, you can apply it to many more scenarios, including one with all the things described in the comment above.

Feel like these navigation components should output unstyled HTML

I agree. I think I will remove the styling that isn't layout oriented. We can always revisit this later.

joshuatf · 2020-07-23T10:45:55Z

Thanks for taking the time to lay out your feedback, @ItsJonQ!

I think I've boiled it down to handful of important requirements:
Supports multiple layers of nesting
Nav items should be "active" aware (e.g. renders bold on current item)
Clicking on nav items can affect things outside the nav menu
Outside changes can affect state of the nav menu

Agreed on all points.

Should gracefully (preferably automatically) handle transition renders (e.g. mounting/unmounting)

Does this mean transition renders for the nav component itself or for the content of the page?

navigate the nav menu one it's own

trigger an action that change something externally (without affecting the nav menu)

or both!

This is where keeping the component “dumb” (presentational) really shines. The navigation component only needs to know which menu items you have and which is active.

With each menu item we can pass a callback parameter so that any component consuming the navigation can decide what happens. This allows the callback to:

Change the page using its own router (e.g, react router).
Change the page using a traditional window location change.
Optionally change the active item.
Run some function that does none of the above.

The advantage to this situation is that it will fit into any existing ecosystem. For the WooCommerce Navigation project this is particularly important since much of the navigation occurs on traditional WordPress screens that have not yet been Reactified.

In terms of Container and Screen components, I agree with @psealock that they're probably outside the scope of this navigational component. I think they're also moving into router territory and a separate discussion is probably warranted on whether or not we have baked in routing for WordPress as a whole or if this uses something underneath the hood like React Router.

Feel like these navigation components should output unstyled HTML.

I tend to agree with this. My only question is how “unstyled” do we want this component to be? Most Gutenberg components contain some level of styling, but I do agree keeping it minimal is important.

psealock · 2020-07-24T01:15:44Z

Update

There is now a feature branch for developing this component incrementally https://github.com/WordPress/gutenberg/tree/feature/navigation.

Here is a list of follow up issues which we can create and add the [Feature] Navigation label. This isn't exhaustive, but enough to get work started.

Remove internal state. (Navigation: Remove internal state psealock/gutenberg#1)
Create a Menu component, ie <Menu name=“primary”> to DRY up the lists and render semantic HTML which is currently div's instead of ul's and li's.
Add transitions.
Add badges to menu items.
Add a way to make menu items urls so a link is rendered.

TimothyBJacobs · 2020-07-24T18:07:42Z

Is there worry that the [Feature] Navigation label might be confused with [Feature] Navigation Screen and the other navigation ( menus ) projects?

ItsJonQ · 2020-07-27T14:58:33Z

Does this mean transition renders for the nav component itself or for the content of the page?

Transition renders for the nav component.

This is where keeping the component “dumb” (presentational) really shines. The navigation component only needs to know which menu items you have and which is active.

I understand the arguments for a purely presentational component set.

Will continue thought with...

I tend to agree with this. My only question is how “unstyled” do we want this component to be?

My idea was for a navigation system that would provide (intelligent) bare-bone primitives.

Let's pretend this are called NavigationStack components.

On top of that, we could provide pre-configured stylized UI pieces (that use NavigationStack). Perhaps these would be called Navigation components.

Having the 2 pieces offers more flexibility for integrating into various scenarios. WooCommerce has a different use-case compared to Gutenberg, where we may need to first use it in FSE.

Presentation only

We could start in this manner. Something that we may not be able to support (in the beginning) may be the animation transitions between arbitrary nested menus.

If we do, I suggest that the component should be broken down into smaller pieces. Example:

Navigation
NavigationItem
NavigationHeader

etc...

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.

As @jameskoster has pointed out, there are varying use-cases. The setup of the nav should be less prescriptive.

Having the pieces allows the user (dev) to compose as needed.

Styles

I would also advise using CSS-in-JS styled components for this UI:
https://emotion.sh/docs/styled

An example of this would be something like Flex:

https://github.com/WordPress/gutenberg/blob/master/packages/components/src/flex/styles/flex-styles.js

Hope this helps!

joshuatf

Thanks for getting the base up and tackling some of that feedback. I think we should get this one merged in so we can unblock some of these issues and continuing iterating.

Also noting that based on @ItsJonQ's feedback above, we'll also need issues for:

Breaking down components into smaller pieces (we can move away from the array of data and primary|secondary potentially.
Converting styles to css-in-js.

I think both of the above items are reasonable, but let me know if you have any differing opinions on the structure here, @psealock.

joshuatf · 2020-07-27T17:00:55Z

packages/components/src/navigation/index.js

+			<div className="components-navigation-items">
+				{ items.map( ( item ) =>
+					! item.isSecondary ? (
+						<Item


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.

psealock · 2020-07-27T18:10:00Z

Thanks for the great feedback again, especially the suggestions on naming and CSS in js. I agree its a good time to merge this into the base branch, create issues and begin pull requests.

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.

I'm having trouble at this time seeing how this would work, but I'm all in favor of being able to remove the secondary/primary logic.

@ItsJonQ @joshuatf would you mind approving this PR?

psealock · 2020-07-27T18:12:23Z

Is there worry that the [Feature] Navigation label might be confused with [Feature] Navigation Screen and the other navigation ( menus ) projects?

Good point @TimothyBJacobs. How about [Feature] Navigation component ?

TimothyBJacobs · 2020-07-27T18:15:18Z

Good point @TimothyBJacobs. How about [Feature] Navigation component ?

That sounds good to me! Could maybe also be [Feature] Admin navigation?

joshuatf · 2020-07-28T06:47:02Z

I'm having trouble at this time seeing how this would work, but I'm all in favor of being able to remove the secondary/primary logic.

I'm not entirely sure we can get rid of it, but if we move towards more atomic components, we can build out the navigation using these components like this:

<Navigation>
  <NavigationHeader>Menu Title</NavigationHeader/>
  <NavigationMenu>
    <NavigationItem>Item from primary</NavigationItem>
  </NavigationMenu>
  <NavigationMenu>
    <NavigationItem>Item from secondary</NavigationItem>
  </NavigationMenu>
</Navigation>

Then the WC nav project is responsible for adding primary/secondary patterns in. Or if we want a little bit more opinion in these components, we might consider using a prop like position to determine the menu so we don't add too many constraints to how these menus are structured.

@ItsJonQ @joshuatf would you mind approving this PR?

I gave approval above, but I guess my permissions are not sufficient to merge in GB. We may need to ping someone on the GB team.

ItsJonQ · 2020-07-29T18:49:50Z

@psealock + @joshuatf HaiiI! Apologies for the delay here.

It sounds like we're ready to start iterating and evolving this component via the WordPress:feature/navigation branch :)

Thanks for getting the ball rolling!

ItsJonQ · 2020-08-06T21:52:31Z

Haiii! Just a heads up, I've started exploring the mechanics of this component in my experimental component library:
ItsJonQ/g2#20

It's not complete yet, but there may be some ideas there that can help with the development of this component within Gutenberg.

Feel free to poke around at the code + demos!

* Initial working tree display * add styles * add styles * dark theme * back button * secondary * BEM style classnames * remove back button styles * add menu: 'primary' data propery * get even more working * remove extra styles * add README * manifest

psealock requested review from ajitbohra, chrisvanpatten and youknowriad as code owners July 22, 2020 02:50

joshuatf reviewed Jul 22, 2020

View reviewed changes

psealock added 12 commits July 24, 2020 11:13

Initial working tree display

0e0bb0c

add styles

016bd69

add styles

c92fb80

dark theme

60fa5f2

back button

647f332

secondary

be4c224

BEM style classnames

ff2a169

remove back button styles

7ff8479

add menu: 'primary' data propery

7ce0d8a

get even more working

0a3d717

remove extra styles

56be028

add README

645018d

psealock force-pushed the add/navigation-component branch from 93ac90c to 645018d Compare July 23, 2020 23:17

psealock changed the title ~~Add Navigation component [Work in progress]~~ Add Navigation component [Feature branch] Jul 23, 2020

manifest

c89e13a

psealock changed the base branch from master to feature/navigation July 23, 2020 23:31

joshuatf approved these changes Jul 27, 2020

View reviewed changes

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

psealock merged commit c1bfc52 into WordPress:feature/navigation Jul 29, 2020

psealock mentioned this pull request Jul 29, 2020

Make nav components more atomic psealock/gutenberg#2

Closed

ItsJonQ mentioned this pull request Aug 6, 2020

Navigation: Add component ItsJonQ/g2#20

Open

griffbrad mentioned this pull request Jun 25, 2021

Global Styles v2: Move prototype from G2 #32923

Closed

7 tasks

griffbrad mentioned this pull request Sep 17, 2021

Components: Update Navigator components — Tracking Issue #34907

Open

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Navigation component [Feature branch] #24107

Add Navigation component [Feature branch] #24107

psealock commented Jul 22, 2020 •

edited

Loading

joshuatf left a comment

joshuatf Jul 22, 2020

psealock Jul 23, 2020

joshuatf Jul 22, 2020

psealock Jul 23, 2020

joshuatf Jul 22, 2020

psealock Jul 23, 2020

joshuatf Jul 22, 2020

psealock Jul 23, 2020

joshuatf Jul 22, 2020

joshuatf Jul 22, 2020

psealock Jul 23, 2020

joshuatf Jul 22, 2020

psealock Jul 23, 2020

ItsJonQ commented Jul 22, 2020

psealock commented Jul 23, 2020 •

edited

Loading

joshuatf commented Jul 23, 2020

psealock commented Jul 24, 2020

TimothyBJacobs commented Jul 24, 2020

ItsJonQ commented Jul 27, 2020

joshuatf left a comment

joshuatf Jul 27, 2020

psealock commented Jul 27, 2020

psealock commented Jul 27, 2020

TimothyBJacobs commented Jul 27, 2020

joshuatf commented Jul 28, 2020

ItsJonQ commented Jul 29, 2020

ItsJonQ commented Aug 6, 2020

Add Navigation component [Feature branch] #24107

Add Navigation component [Feature branch] #24107

Conversation

psealock commented Jul 22, 2020 • edited Loading

Current state

How has this been tested?

Screenshots

Types of changes

Checklist:

joshuatf 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

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

ItsJonQ commented Jul 22, 2020

Identifying Requirements

Router?

Inspiration

Anatomy

Container

Transitioning

Adapting to incoming changes

Screen

Link / navigate

Aesthetics

psealock commented Jul 23, 2020 • edited Loading

joshuatf commented Jul 23, 2020

psealock commented Jul 24, 2020

Update

TimothyBJacobs commented Jul 24, 2020

ItsJonQ commented Jul 27, 2020

Presentation only

Styles

joshuatf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psealock commented Jul 27, 2020

psealock commented Jul 27, 2020

TimothyBJacobs commented Jul 27, 2020

joshuatf commented Jul 28, 2020

ItsJonQ commented Jul 29, 2020

ItsJonQ commented Aug 6, 2020

psealock commented Jul 22, 2020 •

edited

Loading

psealock commented Jul 23, 2020 •

edited

Loading