Try: Use route detection to set active state of links

[joshuatf]

Description

This is an attempt to set the active state based on the current route. It works by watching history changes and updating the active link based on routes that match.

If we go this route, there's probably some follow-ups needed:

• Allowing exact/non-exact matching URLs (just need a prop for this).
• Allowing a consumer to directly override with isActive on an item.
• Testing for new utils

Testing

1. Run npm run storybook:dev.
2. Go to the "Navigation" component.
3. Check that navigating through the links works as expected.
4. Use back/forward in your browser
5. Optionally, navigate to http://localhost:50240/iframe.html?id=components-navigation--default&viewMode=story (to avoid the constraints of the iframe in storybook).
6. Manually manipulate the history in your console. E.g., history.pushState({fdsfds: 'bfdssla'}, 'New Title', '#item-1'); and note the active item is updated.

[psealock]

I'm hesitant to go down this path because it transforms this component from something presentational to a full solution. I see the benefit to offloading this logic from the consuming app, but it comes at the cost of diversity of use cases. @wordpress/components may be used in plugins whose router context may not be react-router and they'd need to add this dependency.

Are there advantages that I'm not considering?

[joshuatf]

I see the benefit to offloading this logic from the consuming app, but it comes at the cost of diversity of use cases.

I'm pretty sure I agree as well. This PR was just meant to be an experiment to test various use cases. I'd rather leave the component open-ended, but on the slight chance that we could cover all use cases in the component, it would be great to internalize this logic.

@wordpress/components may be used in plugins whose router context may not be react-router and they'd need to add this dependency. Are there advantages that I'm not considering?

I think we can skip using the full-blown router and just detect history changes and allow path matching. This would allow the consumer to continue using their own router and this component to watch for changes.

But unfortunately I can't seem to resolve the dependency issue I mentioned in Slack so I'm unclear on how well this works across different routers.

One piece of this I'm concerned with is having multiple extensions who each utilize different routers. Currently we can expose a method to update the navigation active state, but this is one more method that third parties will need to subscribe to. Also in this case, if two plugins overlap on which item should be active, how do we determine the source of truth?

[joshuatf]

@psealock I updated this a bit and pulled out the dependencies in favor of a more simple history detection script.

Pros of this approach:

• Single source of truth - When hooking up this component I noticed that conflicts could easily arise when setting the active item. If multiple plugins are hooking into the navigation, an overly greedy plugin could create conflicts and make it difficult to determine the active item.
• Ease of implementation - Consumers don't need to worry about passing custom logic to the Navigation component. Things work out of the box.
• Works across any router - At least any router that makes use of the native browser history, which I think should cover our bases.

Cons of this approach:

• Maintenance - Some maintenance and testing will be needed to make sure this is cross-browser compliant and for URL detection.
• Custom active items - Currently this does not allow an item to be active without matching the current location. We can mitigate this by allowing a consumer to deliberately mark an item as active with the isActive property on an item.

I started this as an experiment, but I'm starting to lean towards this approach for the pros mentioned above. I'd love to hear any use cases you feel this doesn't work for. My initial thoughts were that there may be cases where location is not the determining factor for status, however, I think these may be edge cases and most navigation items will require a location by nature (even if that's just a hash change).

[gziolo]

Is this PR still relevant? I saw very promising work in #27124 where @Addison-Stavlo and @noahtallen work on similar logic. Do you plan to continue working on this feature?

[joshuatf]

@gziolo This logic has been moved to https://github.com/woocommerce/woocommerce-admin/blob/main/client/navigation/utils.js for the time being. It may be something we eventually look at merging back into GB, but it still needs refining.

Thanks for the link to #27124! We're discussing a similar idea around route/context detection, although we'd need to keep the solution much more open-ended and extensible for 3PDs registering nav items.

Closing this PR for now.