fix NavLink with query string can't be selected correctly #32168

ElderJames · 2021-04-26T07:23:59Z

You've read the Contributor Guide and Code of Conduct.
You've included unit or integration tests for your change, where applicable.
You've included inline docs for your change, where applicable.
There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

PR Title
Summary of the changes (Less than 80 chars)

Remove query string when handling path matching.

PR Description
Detail 1
Detail 2

Addresses #bugnumber (in this specific format)

SteveSandersonMS · 2021-04-26T11:25:13Z

Thanks for proposing this, @ElderJames! There are still some details that would need to be worked out before we could make a change like this.

Back-compatibility. Some developers might be taking a dependency on the link not matching if the querystring is present, so we wouldn't want to just break them by applying this new rule by default. Options:
- We could add a further flag, e.g., bool IgnoreQueryString, so the existing behavior is retained by default
- Or, we could focus more on making NavLink more generally customizable. For example we could change its private bool ShouldMatch to be protected virtual bool ShouldMatch. Then developers who want the kind of change you have here could trivially subclass <NavLink> to make (for example) <NavLinkIgnoreQueryString> with their custom matching rule.
E2E tests are needed for new functionality like this.

If you're interested in proceeding with this, could you make a proposal for how to handle the back-compatibility concern? We can then come to an agreement about that. Once that's resolved and an E2E test is added, it's likely we can take this change.

Thanks very much!

ghost · 2021-04-26T13:05:40Z

Hello. I see that you've just added Needs: Author Feedback label to this PR.
That label is for Issues and not for PRs. Don't worry, I'm going to replace it with the correct one.

ghost · 2021-04-26T13:05:48Z

Hello. I see that you've just added Needs: Author Feedback label to this PR.
That label is for Issues and not for PRs. Don't worry, I'm going to replace it with the correct one.

ghost · 2021-04-26T13:05:56Z

Hello. I see that you've just added Needs: Author Feedback label to this PR.
That label is for Issues and not for PRs. Don't worry, I'm going to replace it with the correct one.

ElderJames · 2021-04-26T15:57:29Z

Thank you for your reply @SteveSandersonMS .

I think adding a IgnoreQueryString flag is better. And this behavior is also quite general.

ghost · 2021-05-06T18:00:51Z

Hi @ElderJames.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

SteveSandersonMS · 2021-05-07T15:55:40Z

@ElderJames If you're interested in implementing the proposed design, including tests, please let us know. Otherwise the bot will soon close this PR.

ElderJames · 2021-05-08T05:10:26Z

Sorry, I was on vacation these days. I will continue to finish this feature.

captainsafia · 2021-05-11T02:25:57Z

@ElderJames Thanks for following up.

The build is failing at the moment since a new public API was added that is not tracked in the text files that we include in the repo.

If you're developing in VS, there is a built-in Roslyn analyzer that you can use to automatically update the text files by right clicking on the warning under the defined symbol.

You can also add the type definition at the end of https://github.com/dotnet/aspnetcore/blob/28acd06c02924b2b9ee20748bf7e38847c94f0c4/src/Components/Web/src/PublicAPI.Unshipped.txt.

SteveSandersonMS · 2021-05-11T09:30:12Z

Sorry, I was on vacation these days. I will continue to finish this feature.

No need to rush! Even if the bot closes the PR it can always be reopened.

ElderJames · 2021-05-17T14:58:17Z

Sorry, I don't really understand how to write this test.😂

captainsafia

Left some comments inline.

With regard to how to write tests, you can leverage our end-to-end testing infrastructure. We have a BasicTestApp that contains a few components with scenarios used for testing.

For your PR, the component you'll want to use is https://github.com/dotnet/aspnetcore/blob/fdabaf00a6720d852ec3120b7bbe4b14de2f3ccf/src/Components/test/testassets/BasicTestApp/RouterTest/Links.razor.

You'll want to add a new NavLink component to that that sets the new parameter.

We exercise that component from Selenium-based tests. For your scenario, the relevant tests are in https://github.com/dotnet/aspnetcore/blob/fdabaf00a6720d852ec3120b7bbe4b14de2f3ccf/src/Components/test/E2ETest/Tests/RoutingTest.cs.

Let me know if you have any other questions!

src/Components/Web/src/Routing/NavLink.cs

captainsafia · 2021-05-17T15:55:56Z

src/Components/Web/src/Routing/NavLink.cs

@@ -173,6 +181,9 @@ protected override void BuildRenderTree(RenderTreeBuilder builder)
        private string? CombineWithSpace(string? str1, string str2)
            => str1 == null ? str2 : $"{str1} {str2}";

+        private string RemoveQueryString(string path)
+            => path.Split('?')[0];


Just to be a little bit more defensive here can we check to see if the string contains a ? character first and then produce a substring only if it does?

Co-authored-by: Safia Abdalla <safia@safia.rocks>

ghost · 2021-05-28T03:00:39Z

Hi @ElderJames.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

PaulSinnema · 2021-10-03T09:25:25Z

Don't close this bug. It's an ugly one. Please solve it.

ghost · 2021-10-03T09:25:28Z

Hi @PaulSinnema. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

ElderJames requested a review from a team as a code owner April 26, 2021 07:23

ghost added area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member labels Apr 26, 2021

ElderJames force-pushed the fix/navlink branch 3 times, most recently from 2449934 to 0228e04 Compare April 26, 2021 07:31

SteveSandersonMS added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Apr 26, 2021

ghost added pr: pending author input For automation. Specifically separate from Needs: Author Feedback and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Apr 26, 2021

SteveSandersonMS added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Apr 26, 2021

ghost removed the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Apr 26, 2021

SteveSandersonMS added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Apr 26, 2021

ghost removed the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Apr 26, 2021

mkArtakMSFT assigned captainsafia Apr 26, 2021

ghost added the stale Indicates a stale issue. These issues will be closed automatically soon. label May 6, 2021

fix NavLink with query string can't be selected correctly

db10c6b

add IgnoreQueryString flag

28acd06

ElderJames force-pushed the fix/navlink branch from 0228e04 to 28acd06 Compare May 8, 2021 05:34

ghost removed stale Indicates a stale issue. These issues will be closed automatically soon. pr: pending author input For automation. Specifically separate from Needs: Author Feedback labels May 8, 2021

captainsafia added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label May 11, 2021

add type definition

9d4d2f7

captainsafia reviewed May 17, 2021

View reviewed changes

Update src/Components/Web/src/Routing/NavLink.cs

fbf80bb

Co-authored-by: Safia Abdalla <safia@safia.rocks>

ghost added the stale Indicates a stale issue. These issues will be closed automatically soon. label May 28, 2021

ghost closed this Jun 1, 2021

SteveSandersonMS mentioned this pull request Jun 16, 2021

Blazor QueryString enhancements - design proposal #33338

Closed

This was referenced Mar 31, 2022

When using a querystring with Blazor apps the the default NavItem is not selected. #31312

Closed

Add IgnoreQueryString parameter to NavLink #40990

Closed

ilonatommy mentioned this pull request Jan 16, 2025

Ignore query/fragment in ShouldMatch of NavLink by default but allow overriding ShouldMatch #59903

Merged

javiercn mentioned this pull request Mar 24, 2025

Api proposal for NavLink.ShouldMatch #61137

Closed

This pull request was closed.

fix NavLink with query string can't be selected correctly #32168

fix NavLink with query string can't be selected correctly #32168

Uh oh!

Conversation

ElderJames commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveSandersonMS commented Apr 26, 2021

Uh oh!

ghost commented Apr 26, 2021

Uh oh!

ghost commented Apr 26, 2021

Uh oh!

ghost commented Apr 26, 2021

Uh oh!

ElderJames commented Apr 26, 2021

Uh oh!

ghost commented May 6, 2021

Uh oh!

SteveSandersonMS commented May 7, 2021

Uh oh!

ElderJames commented May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

captainsafia commented May 11, 2021

Uh oh!

SteveSandersonMS commented May 11, 2021

Uh oh!

ElderJames commented May 17, 2021

Uh oh!

captainsafia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

captainsafia May 17, 2021

Choose a reason for hiding this comment

Uh oh!

ghost commented May 28, 2021

Uh oh!

PaulSinnema commented Oct 3, 2021

Uh oh!

ghost commented Oct 3, 2021

Uh oh!

Uh oh!

ElderJames commented Apr 26, 2021 •

edited

Loading

ElderJames commented May 8, 2021 •

edited

Loading