Navigation Component: Custom item content #25281
Conversation
psealock
requested review from
ajitbohra,
chrisvanpatten and
youknowriad
as code owners
psealock
added
the
[Feature] Navigation Component
|
Size Change: +97 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
| </Button> | ||
| { navigateToMenu && <Icon icon={ chevronRight } /> } | ||
| </Button> | ||
| ) } |
There was a problem hiding this comment.
This approach has one caveat: custom components would need the whole external state to handle internal navigation/item activation (see the storybook change).
I'd love to keep the component as self-contained as possible by default, leaving the external state as an optional feature, only used if strictly needed.
What if we did some kind of hybrid, and wrap custom components in a div with just the onClick handler?
E.g.
{ children && <div onClick={ onItemClick }>{ children }</div> }
{ ! children && <Button /> }This would have some pros...
- Clicking the item would activate it without the need of an external state.
- Consumers could define an
onClickforNavigationItemif needed, and a different one for the custom component. navigateToMenuwould work as expected.
and cons...
- There would be no way to prevent
setActiveItemon click — unless defining thehrefprop... Maybe we could have another prop to optionally disable the activation behaviour? (Sounds overkill 🤔 ) navigateToMenuon custom components would not render the chevron icon. Not a big deal, but it's an inconsistency.
There was a problem hiding this comment.
This approach has one caveat: custom components would need the whole external state to handle internal navigation/item activation (see the storybook change).
I do agree that this is a problem and for the WC nav we're building it makes things pretty complicated. How do we check and control the active state when we have menu items all with various logic to determine active status:
- Traditional wp links
- WooCommerce Admin links
- Other router-based links
- Third party plugin links
- External links
Clicking the item would activate it without the need of an external state.
I like the simplicity of this and it works for most cases. One small issue is that clicking does not necessarily guarantee that an item will become active. One more common case for this might be external links.
I do have an open PR that uses the current route as a single source of truth for determining active state (would need a rebase if we went that direction). #24985
Pros
- Single source of truth for active state.
- External components can be used without click handlers.
- Works across any type of router.
Cons
- Far more opinionated on what is deemed active and requires a route change (or at least a hash change) to trigger changes to the active state of items.
- Lots of testing needed to confirm this works for all use cases.
There was a problem hiding this comment.
Thanks for the feedback @Copons!
What if we did some kind of hybrid, and wrap custom components in a div with just the onClick handler?
E.g.
The linter will complain about a div with an onClick because it will need role="button" applied as well, which sort of defeats the purpose because semantically an onClick should only be applied to buttons.
I suppose if you have setActiveItem called internally as well as updated from top level props via a change in the url you'd just get an unnecessary render. However, this seems like an indication that something isn't right in the underlying code.
Thanks also @joshuatf
One small issue is that clicking does not necessarily guarantee that an item will become active. One more common case for this might be external links.
In the case of external links, no navigation actually occurs in the originating page, so I think this is the expected behaviour.
I do have an open PR that uses the current route as a single source of truth for determining active state
This logic is going to be needed by all applications using the Nav component, so I do see the value in bring that in-house. I'm hesitant to advocate for this because it goes beyond the responsibility of the Nav component, which is to render links in a hierarchy. I suppose it's a question of whose responsibility this is. My opinion is why not make a dumb component now and look into adding this later?
There was a problem hiding this comment.
In the case of external links, no navigation actually occurs in the originating page, so I think this is the expected behaviour.
In the example above with an internal state, the onClick would still fire and would mark this item as "active" which I would consider unexpected behavior since the originating page did not navigate.
I'm hesitant to advocate for this because it goes beyond the responsibility of the Nav component, which is to render links in a hierarchy. I suppose it's a question of whose responsibility this is.
I get this and also am a little conflicted about it, but if every consumer is using this logic, I think it should be coupled with the nav. If we see value in the route detection, but can see use cases for setting active state without it, then I agree that separation is warranted.
My opinion is why not make a dumb component now and look into adding this later?
I'd argue that this logic is necessary to make the component usable. While I like the idea of a dumb component, if every consumer is going to write their own route detection logic, why not add it sooner rather than later and make this component more readily usable?
The alternative is just allowing the isActive prop to passed to menu items and having consumers pass their own route detection to any registered items.
I think we should revisit this requirement. Let's look at WooCommerce's needs for this component, as pointed out above by @joshuatf :
These are all links. As such, every single variation will render an On the other hand, lets examine the use case for FSE, #23939. It appears that every click on a side bar item also represents a navigation. Even the custom components that render a preview on hover will ultimately be wrapped in an anchor tag as they too will represent some kind of url navigation.
Keeping an internal state such that the Nav component works "out of the box" is catering to a solution without a use case. Even the more ambitious uses, such as the entire wp-admin sidebar is also merely a collection of links. Now, with the question at hand in #25281 (comment), we are contemplating complicating the API or even bringing the entire state in-house for the sake of a use case that has not yet materialized. What if we freed ourselves from an internal activeItem state?
|
psealock
force-pushed
the
add/Nav-component-item-content
branch
from
4c00203 to
2c3e9c8
Compare
|
fb6cbe7 changes the story to illustrate an example with a Link as custom component. I don't think we need to rip out the internal state, but its become apparent to me that we've been thinking about this Nav component in terms of buttons because we've been developing it in Storybook instead of a real application. As a result, how to handle the variety of links has been an after thought instead of a primary focus. |
This makes total sense, developing in the Storybook really went to my head! I'll recap a little so that we are all on the same page.
This said, I think there's just no reason to keep the Am I way off base or am I slowly getting this right? 😅 |
@Copons I believe you've got it; looks correct on all bullet points.
Agreed.
👍
I'm not sure I love the idea of every consumer rewriting the same logic to check the routes against URLs, but yes that would work and on the upside it keeps this component very "dumb." |
psealock
force-pushed
the
add/Nav-component-item-content
branch
from
c003a14 to
13038b9
Compare
|
Thanks for the feedback team. UpdateSee 13038b9
|
There was a problem hiding this comment.
LGTM!
Just FYI I've pushed a commit that removes onActivateItem from the readme, but since it doesn't change the actual code, I'll approve this anyway! 🙂
Thank y'all so much for bearing with me on this.
For as much as I liked conceptually to keep the whole thing self-controlled, it just didn't make sense in real life, and even worse, it could get in the way of various use cases.
Fixes #25244
Description
Instead of rendering
NavigationItemcontent inside a button, make it completely up to the consumer to handle contents if children are supplied. Normal navigation items and navigation links can be designated via props and will be rendered with a button. Otherwise, if a child element is supplied, render that and leave controlling it to the consumer.This is a first stab at a solution. I think the level of control given to the consumer is up to debate. How much should the component handle? How much to be offloaded to the consumer?
From the WooCommerce perspective, custom item content will be used to pass in internal links for navigation without a page refresh: ie, a
Linkcomponent tied to the page's React Router instance. There won't be a need for passing anonClickmethod to specifyactiveItemnor will that logic be required internally viasetActiveItem. Instead a click will cause React Router to change locations and the top levelNavigationwill be passed theactiveItemprop via the new url.How has this been tested?
npm run storybook:devScreenshots
Types of changes
Changes the API of Navigation component such that
NavigationItemrendering children handle all responsibility of navigation.Checklist: