Ignore query/fragment in ShouldMatch of NavLink by default but allow overriding ShouldMatch by ilonatommy · Pull Request #59903 · dotnet/aspnetcore (original) (raw)

@SteveSandersonMS in case you have strong opinions about it. I think the original intent of Match.Prefix and Match.All was to match based on the path, and that we didn't account for the query string or the hash at the time.

The original intent was focused on the case where you have links to

• /
• /something

... and you want:

• / to be treated as the current URL only when you're at the URL / (so you'd use Match.All)
• /something to be treated as the current URL when you're at /something or /something/{id} (so you'd use Match.Prefix)

Treating Match.All as including the query/hash, as we do today, isn't ideal for this case. It would be unusual to want a link to /?page=2 not to highlight the link to /. So the proposed new behavior of ignoring the query/hash seems like an improvement in this specific (and common) case.

There is possibly a case where the change would be bad, e.g., if you had a tabview with links like:

• /mypage?tab=first
• /mypage?tab=second

Now if <NavLink> ignores querystring by default, you'd lose highlighting on the links, and would have to subclass NavLink to get the correct highlighting back. At least I think so - please correct me if I've got this wrong. BTW I have built URL/link structures like this in Blazor before but can't estimate how common it is externally.

If we were building things from scratch here I'd be comfortable defaulting to "ignore query/hash" and force people who want distinguishable links to ?tab=first and ?tab=second to do extra work. However, taking this as a breaking change is a harder call to make, even with an appcompat switch. I think it's reasonable for you, @lewing, @ilonatommy etc to make that call (and if you do choose to take the breaking change, just ensure it's called out through whatever process we advertise such breaking changes).