Add custom nav link props to Navigation component

[joshuatf]

Description

NOTE: Branched from add/nav-badges and will need a rebase pending its merge.

Allows passing the link tag name as well as additional props to this component. Allows consumers to bring their own routing components and link tags.

Screenshots

Testing

1. Run npm run storybook:dev.
2. Visit the Navigation component.
3. Check that the newly added "Custom link" works by opening a new browser tab.

[psealock]

Very nice. I noted an opportunity to simplify logic

[psealock]

Can we make an assumption here that if an href is found, use a anchor tag?

[psealock]

Now that I think about it, Button already has this logic built in.

gutenberg/packages/components/src/button/index.js

Line 67 in 375eaa0

const Tag = href !== undefined && ! trulyDisabled ? 'a' : 'button';

[joshuatf]

Ya, button already includes this logic; the use of a here is purely for demonstration, though we could easily swap this with another tag if you think it would be more appropriate.

Using linkTag allows us to pass a prop like the Link component or some other custom component to integrate with various routers.

[psealock]

There are 3 types of nav actions:

• Button with a callback
• External link
• Internal link

To differentiate between the first two can be as simple as the presence of an href in the config data. Should the onClick callback be disabled in this case? Does the External Link component offer any functionality here?

The 3rd type provides navigation in whatever context of the consuming app's routing. A callback could be sufficient as you can do something like getHistory().push( /** something */ );. But what about passing a component like <Link /> from wc-admin? Maybe NavigationMenuItem could have link prop or something. What do you think?

[joshuatf]

To differentiate between the first two can be as simple as the presence of an href in the config data.

I think the Button component adequately covers this.

Should the onClick callback be disabled in this case?

I'm in favor of allowing both. The use case here might be a link click, but also wanting to fire an event, such as tracking.

Does the External Link component offer any functionality here?

Seems like it does offer some a11y support, but I would let consumers use this in the linkTag if they feel inclined to and just defer to default Button behavior where possible.

But what about passing a component like from wc-admin? Maybe NavigationMenuItem could have link prop or something. What do you think?

Agreed- that's the intention of the linkTag 😄 linkTagProps allows passing additional parameters like to or anything necessary for a passed component's routing.

[psealock]

Can you rebase this one now that the two previous PRs are merged?

[psealock]

To differentiate between the first two can be as simple as the presence of an href in the config data.

I think the Button component adequately covers this.

Indeed it does. If you let Button handle all the logic with this change, not only doe Button apply the correct presentational styles, but it respects the target: '_blank', and renders sematically with an <a /> tag.

        return (
                <MenuItemUI className={ classes }>
-                       <LinkTagName
+                       <Button
                                className={ classes }
                                href={ href }
                                onClick={ handleClick }
@@ -56,7 +56,7 @@ const NavigationMenuItem = ( props ) => {
                                </Text>
                                { badge && <BadgeUI>{ badge }</BadgeUI> }
                                { hasChildren ? <Icon icon={ chevronRight } /> : null }
-                       </LinkTagName>
+                       </Button>
                </MenuItemUI>
        );
 };

In 8864e06 I take it one step further by adding a LinkComponent prop for passing in a custom component. I went ahead and pushed to the branch because it creating a new PR for just that change hides it under the psealock namespace. What do you think? Feel free to revert the commit or push new changes.

[joshuatf]

Indeed it does. If you let Button handle all the logic with this change, not only doe Button apply the correct presentational styles, but it respects the target: '_blank', and renders sematically with an tag.

Just for visibility from above:

Ya, button already includes this logic; the use of a here is purely for demonstration, though we could easily swap this with another tag if you think it would be more appropriate.

In 8864e06 I take it one step further by adding a LinkComponent prop for passing in a custom component.

The linkTag I previously added allowed passing in a custom component, so it looks like 8864e06 renames linkTag to LinkComponent. I think the renaming here is good, but we should keep my old implementation for the tag instead of separating the two and having to pass props twice.

[joshuatf]

Missing href in here. I think the old implementation simplifies the props logic here a bit.

[joshuatf]

Nice addition 👍

[joshuatf]

@psealock Added back in the link component tag in 2b71528

[psealock]

I like the composite approach here. I did realize after the fact that your approach did indeed allow passing of a custom component. I think we will need some good documentation around LinkComponent, and the entire config for that matter.