NavLink not highlighting when on sub pages

[razzeee]

Hey there,

this is more of a question, as I'm pretty surprised this doesn't work (like this).
I thought writing a test would be the sane thing to check if I'm really not doing anything wrong and at the same time to explain, what I'm trying to do.

Cheers

---

Release Notes

The <NavLink> component takes a new prop, matchSubPaths that will make it apply the activeClassName className also for sub path matches. So if you have a NavLink to /posts it will also add the active class name when the user is on, for example, /posts/2 or /posts/new
The useMatch hook takes the same parameter and will then also match sub paths.

matchPath is no longer exported from @redwoodjs/router. If you're using it we recommend you use the useMatch hook instead. If useMatch doesn't cover what you were using matchPath for, please let us know.

[jtoar]

Hey @razzeee, thanks for writing a test, it makes what you're looking for super clear. Yeah it doesn't work like this—NavLink match the URL exactly. We added a prop, activeMatchParams, to match against search params, but didn't consider matching against route params (here's the PR #2683). This seems like a more-than-valid use case though. I'll confirm with the team that it's ok to move forward. If so, now that you've contributed a test, would you want to fix it by contributing the implementation? No pressure!

[razzeee]

I've pushed an iteration, not too proud of it and probably multilpe things to improve. Not sure about that whole params part for e.g.

And also not sure if the logic should also allow matching for inbetween paths like /products/category1/page/1 where you might want to match for category1. But not sure, if that's a real usecase.

[Tobbe]

Now that we have so many arguments to this function, could you please switch to an options object instead? Either for all of them, or for just the last two.

[razzeee]

Wouldn't that change the api? Is it safe to assume, that this is only used internally?

[Tobbe]

That's a good shout! The "public version" is supposed to be the useMatch hook. But we are also exposing this function, so while it's ment to be internal only, it really isn't.

Let's leave it as it is for now and I'll ask the rest of the team. We are about to release RW v4 fairly soon. So if we are to introduce a breaking change that would be a good time to do it :)

[Tobbe]

I switched to an options object, but I also kept the old way of calling the function, but with a deprecation notice.

The change that this PR introduces is something I need in one of my own projects now, so keen to get it merged. And always easier to merge without breaking changes :D

[Tobbe]

I've pushed an iteration, not too proud of it and probably multilpe things to improve. Not sure about that whole params part for e.g.

It's a great first iteration. Thanks for putting in the time and effort! I did have a few comments, but hopefully nothing too difficult to address. Also don't hesitate to tell me I'm wrong about any of the comments 🙂

And also not sure if the logic should also allow matching for inbetween paths like /products/category1/page/1 where you might want to match for category1. But not sure, if that's a real usecase.

I say we skip this for now, and then come back and add support for that when/if someone explicitly asks for it and shows a good usecase

[Tobbe]

I pushed two more tests. They are currently failing. I think they should pass, but please tell me if you don't agree.

I've also asked about the breaking change to matchPath and I'm now waiting for an answer

[razzeee]

Fixed the failing tests and added another one, that's relevant. The params part still feels fishy to me.

[pantheredeye]

Hey @razzeee @Tobbe some interest in this came from discord today

Hi! I have a custom link component that uses NavLink and maps to routes dynamically. I would like to keep the activeClassName styles apllied to the main navigation when the user has navigated to a child route. but I'm unable to find a way to pass that data into activeMatchParams. Can anyone offer advice?

Any thoughts on time to merge?

[jtoar]

I need to review; so just waiting on me. I’ll get to it soon!

[jtoar]

My thoughts are...

• I think the name of the prop on NavLink, matchChildren, could be a bit confusing because it's a React component, and children has another meaning in that context.

  Maybe we just double up on matchChildRoutes? That name seems to do the job for the hook—was there a reason to change it for the component?

• This is nitpicking in absence of a style guide, but I'm not sure if everything in matchPath should be an object (@Tobbe you originally suggest "just the last two"—I'd prefer that basically).

  matchPath is supposed to match the path prop of a route to a location pathname. The rest of those things are options. So it's probably just time for an options obejct:

  matchPath('/user/{org:String}/{username:String}', '/user/redwood/tom', { paramTypes, matchChildRoutes })

  Ideally we stop exporting literally everything from the web package so that we can make these kind of changes with impunity. But alas.

If we agree I'm happy to make these changes.

[razzeee]

* I think the name of the prop on `NavLink`, `matchChildren`, could be a bit confusing because it's a React component, and `children` has another meaning in that context.
  Maybe we just double up on `matchChildRoutes`? That name seems to do the job for the hook—was there a reason to change it for the component?

Agreed, I think I renamed stuff to matchChildRoutes as I found it confusing/to be a better name, but did not change it all the way up. I probably was only thinking of the local problems at that moment

[Tobbe]

My thoughts are...

Thanks a lot for your review Dom. It sparked some other ideas for me. Please let me know what you think.

• I think the name of the prop on NavLink, matchChildren, could be a bit confusing because it's a React component, and children has another meaning in that context.
  Maybe we just double up on matchChildRoutes? That name seems to do the job for the hook—was there a reason to change it for the component?

Good point about children. I switched to matchSubRoutes to get rid of it altogether. But happy to switch to matchChildRoutes if you prefer that.

• This is nitpicking in absence of a style guide, but I'm not sure if everything in matchPath should be an object (@Tobbe you originally suggest "just the last two"—I'd prefer that basically).
  matchPath is supposed to match the path prop of a route to a location pathname. The rest of those things are options. So it's probably just time for an options obejct:

  matchPath('/user/{org:String}/{username:String}', '/user/redwood/tom', { paramTypes, matchChildRoutes })

  Ideally we stop exporting literally everything from the web package so that we can make these kind of changes with impunity. But alas.

If we agree I'm happy to make these changes.

To keep backwards compatibility I introduced a new method for the new functionality. So no need for an options object. But I can introduce one for the new method I created (with just one option for now) if we want to make it more future proof. Keeping it the way it is makes both exported functions have the same signature, which I like.

[jtoar]

I'd prefer matchChildRoutes over matchSubRoutes; it's just matchChildren that I thought could be confusing. And for the second point, I like the signature you have here:

const internalMatchPath = (
  route: string,
  pathname: string,
  {
    paramTypes,
    matchChildRoutes,
  }: {
    paramTypes?: Record<string, ParamType>
    matchChildRoutes?: boolean
  }
) => {
  ...
}

That's two options isn't it? Maybe this isn't the new one you created though since it's prefaced with internal.

[Tobbe]

That's two options isn't it?

I should have been more clear. I meant the new exported function. It takes the same parameters as the original function.

Should I rename matchSubPath to matchChildPath?
And since the original function is named matchPath, should the parameter be named matchChildPaths?

@razzeee I value your input on naming too 🙂

[Tobbe]

Ideally we stop exporting literally everything from the web package so that we can make these kind of changes with impunity. But alas.

At first this was my reaction too. But thinking about it more I actually don't mind enabling advanced usage of Redwood with these functions.

[Tobbe]

We talked about this PR on a team meeting today.

• Rename prop to matchSubPaths (both on NavLink and hook)
• Don't export matchPath from index.ts
• Have only one function, matchPath(route, pathname, { paramTypes, matchSubPaths }
• Add release note copy in PR description

Still not 100% clear on if we can release this in a minor, or if it'll have to wait until v4 (RC out next week, GA in January)

I'll start working on those points asap