Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: [FluentNavLink] An issue where empty strings were not allowed in Href #2722

Merged
merged 4 commits into from
Sep 28, 2024

Conversation

dannyldj
Copy link
Contributor

Pull Request

πŸ“– Description

Fixed FluentNavLink not working correctly when Href was empty

πŸ‘©β€πŸ’» Reviewer Notes

So simple PR

πŸ“‘ Test Plan

Verify that clicking on a FluentNavLink correctly navigates to the root page when the Href of the FluentNavLink is empty.

βœ… Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new component
  • I have modified an existing component
  • I have validated the Unit Tests for an existing component

vnbaaij
vnbaaij previously approved these changes Sep 26, 2024
@vnbaaij vnbaaij dismissed their stale review September 26, 2024 18:41

Not wanted behavior

@vnbaaij
Copy link
Collaborator

vnbaaij commented Sep 26, 2024

There is a reason we do not want to have empty Hrefs. This is done on purpose so you can have entries in the FluentNavMenu that don't necessarily go somewhere but do invoke an action. Like in the https://www.fluentui-blazor.net/NavMenu#navigation-examples:

<FluentNavLink OnClick="OnClick" Icon="@(new Icons.Regular.Size20.LeafOne())">Item 3.1</FluentNavLink>

If you still need to have a link that goes to the homepage, you can use asa value either "/" or "./". The latter can be used in case of using a proxy and using PathBase

See also #1654

@dannyldj
Copy link
Contributor Author

dannyldj commented Sep 26, 2024

The original NavLink component accepts an empty string, and the link below recommends an empty string as a way to navigate to the root page.
https://learn.microsoft.com/ko-kr/aspnet/core/blazor/fundamentals/routing?view=aspnetcore-8.0#navlink-component

Example

<div class="nav-scrollable" 
    onclick="document.querySelector('.navbar-toggler').click()">
    <nav class="flex-column">
        <div class="nav-item px-3">
            <NavLink class="nav-link" href="" Match="NavLinkMatch.All">
                <span class="bi bi-house-door-fill-nav-menu" 
                    aria-hidden="true"></span> Home
            </NavLink>
        </div>

+       @foreach (var name in GetRoutableComponents())
+       {
+           <div class="nav-item px-3">
+               <NavLink class="nav-link" 
+                       href="@Regex.Replace(name, @"(\B[A-Z]|\d+)", "-$1").ToLower()">
+                   @Regex.Replace(name, @"(\B[A-Z]|\d+)", " $1")
+               </NavLink>
+           </div>
+       }

    </nav>
</div>

@dannyldj
Copy link
Contributor Author

dannyldj commented Sep 26, 2024

Since that property is nullable, it will work as intended, as long as you don't make the mistake of declaring the Href to be an empty string and throwing in an OnClick callback.

@vnbaaij vnbaaij self-requested a review September 28, 2024 13:03
@vnbaaij
Copy link
Collaborator

vnbaaij commented Sep 28, 2024

Tested on the NavMenuDefault example and it indeed works as expected. Merging it in. Thanks!

@vnbaaij vnbaaij enabled auto-merge (squash) September 28, 2024 13:05
@vnbaaij vnbaaij merged commit 07cab03 into microsoft:dev Sep 28, 2024
4 checks passed
@dannyldj dannyldj deleted the fix/FluentNavLink branch September 28, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants