Active state not working correctly on links with query strings

[AndrewIngram]

Scenario

I have a link in my global header to page with a filterable list of objects. The link includes a query parameter, because the starting state of the list is to already have a filter applied. The link looks something like this:

<Link 
  className="menuitem" 
  activeClassName="active" 
  to={{pathname: '/tickets', query: {status: 'open'}}}>Tickets</Link>

When I click the link and the page loads, the active class is correctly added to the link.

Problem

When I change the filters, e.g. changing the path to just /tickets or /tickets?status=closed the active state is removed from the links.

What I expect

I expect query strings to be ignored for all consideration of what is the active path. Links behave as expected if they link to the naked pathname, rather than one with a query string (i.e adding filters preserves the active behaviour), but break when the link itself contains a query.

Versions affected

Only observed with 2.0+, but no reason to assume the problem didn't exist before.

[timdorr]

This is operating as designed and tested. We expect the full location to match, including the query string.

It's actually more useful this way. Otherwise, how would you go about having links to the Open / Close state and mark either as the currently active one?

[AndrewIngram]

I'd have to solve that problem myself anyway, and already have (this isn't the first time I've built a complex faceted search feature). Because as soon as you care about more than one query parameter (which I do), whether a filter is "active" can't be determined without looking at the individual parameters - exact marching on the full URL won't work.

Aside: if you're asking me a follow-up question, shouldn't you wait for my reply before closing the ticket?

On 13 Apr 2016, at 19:29, Tim Dorr notifications@github.com wrote:

This is operating as designed and tested. We expect the full location to match, including the query string.

It's actually more useful this way. Otherwise, how would you go about having links to the Open / Close state and mark either as the currently active one?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

[taion]

It's tricky here. Ultimately I think we have a relatively simple API in <Link> that works well for most common use cases. You're correct that custom use cases like yours where the link activity and the link target are different will require extra code.

I'm not sure what we can necessarily do here, though.

[ryanflorence]

For every person who wants it to work the way discussed here (if the segment of the query matches, it's active) there's another who doesn't want that to be active, but only if the entire query matches.

In these situations, we ask "what does the browser do?".

a:visited { color: purple; }

<a href="?foo=bar">bar</a>
<a href="?foo=baz">baz</a>

If you visit ?foo=bar it will turn red, but ?foo=baz will not.

We think this is the path of least surprise.

[timdorr]

Aside: if you're asking me a follow-up question, shouldn't you wait for my reply before closing the ticket?

It was rhetorical. Sorry if it came off snarky. There's really a few different ways to define "active"-ness of a URL. We've chosen one that works well for most people and the decision is made. There's not really anything actionable here.

What would make a better issue is the discussion and consideration of more expressive "active" checking. For instance, having props like activePathClassName, activePartialPathClassName, activePartialQueryClassName, etc etc to check various partial or exact "active" states. There is probably a more concise and flexible means of doing that. If there's a specific suggestion, that would be greatly appreciated.

[AndrewIngram]

@ryanflorence I don't think we should automatically use the state of visited links as an example to follow.. Active route functionality has no existing semantics in browsers, mapping it to something else that may not be analogous is an error in my mind.

This is functionality that can be ignored and re-implemented by users of react-router (I assume, since i'm being told this is what I have to do). The fact that it can be implemented on top of the router, suggests to me that the only way it can justify being in the core is if everyone works in a way that you can argue solves nearly everyone's problems. Otherwise it's dead file-size that we can't opt out of.

To that end, I'd like to ask what use cases active route indicators actually address, two spring to mind, and I'm not convinced they're fulfilled by the current implementation:

• A user needs some indication of which "section" or "page" of a site they are on. Following best practices for URL semantics, the path segments indicator the resource, query strings indicate options which manipulate the display of that resource (filters, sorting, paging, and frustratingly, tracking parameters). Following this practice, a query string doesn't change what resource you're looking at. The example i've provided and suggested as being broken, fits this use case - I want my nav link styling to indicate that a user is on a certain section of the site. I just happen to link directly to a pre-filtered list as a UX decision to save time.
• A user is on a list of results, and needs to see what filters are selected, and what page they're on - Again, using best practices, query parameters are used to fulfil the URL part of this. We'd consider one of the links that controls this (let's say a facet link, or a pagination link), to be active when the query for the page includes the key/value pair represented by the link. Things like default filters and page 1 of pagination add further edge cases. Because this behaviour can end up being quite complex, i'd never expect my routing library to be responsible for telling me whether a filter option is currently active. Compared to the complexity of building an appropriate to prop for a Link with lots of filters, working out whether a link is active is trivial, and will usually be done at the same time. This is the use case alluded to by @timdorr in his first comment, and I think it's too complex to expect it to be supported by the default behaviour of the router.

(Note, I appreciate my mentioning of best practices is somewhat nebulous, but i've been a URL design nut for about 10 years, and haven't seen much to contradict what i'm saying)

Basically, I think neither of these use cases are wholly supported by the current solution, but I strongly suspect the first one is a more universal requirement for building sites, and it's also the use case closest to being fully supported - with the exception of the edge case I've presented in this ticket. If there are common use cases i've missed, i'd love to hear them - if i'm wrong, I want to know why :)

(Another aside: should this whole feature have a different name? Because "active" already has a defined meaning when it comes to link states.)

[taion]

I can tell you that as a user of this library, the first time I hit the functionality for determining active state for queries on <Link>, that this behavior was exactly what I expected.

Additionally, at this point, it'd be quite expensive for us to introduce a breaking change to the semantics of active state on query state on <Link>, even to the extent that we want to make such a change.

As such, why don't we talk about ways to better support your use case? One option I'm considering is backporting a few changes from React-Router-Bootstrap into here.

The one change relevant to you would be to accept an active prop (forget the naming for now) that overrides the automatically calculated active state.

Another option might be to factor out reusable logic (the click handler, mostly) in <Link> and allow you to build your own.

Thoughts?

[AndrewIngram]

I'm actually a little confused, because when I discussed this with you in Discord a few days ago, you indicated that the behaviour I'm expecting here is what was already supported, however that's not important. I still want to know what use cases I'm missing, because if we're going for unsurprising, I find the current implementation surprising. But in the interests of this conversation going somewhere, let's look at your suggestions.

I think any solution has meet a couple of criteria:

• It needs to be easy to determine what route is active based on whatever pattern I choose. There probably aren't that many different ways of doing this, but who knows?
• The size of my code bundle shouldn't suffer from a solution i'm not using - i'm happy to let tree-shaking take care of this. But ultimately, if I'm solving this from scratch for myself, I don't want any mention of react-router's active functionality making it to my production build.

I don't mind writing my own Link component, as long as it's trivial to do so and i'm not copy-pasting logic. I already wrap every DOM and 3rd party component to inject CSS Modules, so this isn't creating work I don't already do. Conceptually it feels cleaner than adding yet another prop to Link, because I believe in small-focused components rather than ones with a billion configurable properties.

In terms of determining active route, I like @timdorr's suggestions, but I'd prefer being able to perform those checks via the router or history objects rather than as className props (who knows, it may affect something other than what classes get applied to the link). It feels like I need some of the logic that React Router provides, but some way of specifying which parts of the URL I care about when it comes.

[taion]

I'm not sure what you mean.

If you think about what's involved here, this is going to look effectively like calling isActive with a different location descriptor on your end. I don't think we want to change the semantics of isActive.

This line of code: https://github.com/reactjs/react-router/blob/b5175b15bf0999e5fb151f57a8126aca3537a7c9/modules/Link.js#L119 is all that's involved in the link active state determination. Nothing else would change here, so code size ought not be a concern here.

[AndrewIngram]

What else is router.isActive used for?

[taion]

It's exposed as an API on the router. I use it directly in a few places (via <LinkContainer>) when my logical site mapping doesn't line up with my URL/route scheme.

[AndrewIngram]

Okay, based on what you've said, it seems the right approach is pulling out the bulk of Link so it's easy to use in an alternative component. No other changes would be required.

[taion]

Which parts of <Link> do you specifically not want?

[AndrewIngram]

Well it seems i'd have to write my own version of of the render method to create a different descriptor, which looks like a lot of code copying for such a tiny change.

[taion]

The idea would be that we accept active as a prop to <Link>, and let that take precedence over the computed value.

[AndrewIngram]

That works, and if it's the simplest approach to implement I'm happy. I'm not convinced it's the best solution, but i'd live with it for now.