Fixed NavLink Issue (#10734)

Co-authored-by: Bilal Kazmi <[email protected]> Co-authored-by: Bilal Kazmi <[email protected]> Co-authored-by: Matt Brophy <[email protected]>
remix-run · Oct 31, 2023 · e6ac5f0 · e6ac5f0
1 parent 40701e5
commit e6ac5f0
Show file tree

Hide file tree

Showing 5 changed files with 132 additions and 7 deletions.
diff --git a/.changeset/itchy-items-hang.md b/.changeset/itchy-items-hang.md
@@ -0,0 +1,5 @@
+---
+"react-router-dom": patch
+---
+
+Fix `NavLink` `active` logic when `to` location has a trailing slash
diff --git a/contributors.yml b/contributors.yml
@@ -31,6 +31,7 @@
 - bbrowning918
 - BDomzalski
 - bhbs
+- bilalk711
 - bobziroll
 - BrianT1414
 - brockross

diff --git a/docs/components/nav-link.md b/docs/components/nav-link.md
@@ -93,12 +93,14 @@ You can pass a render prop as children to customize the content of the `<NavLink
 
 The `end` prop changes the matching logic for the `active` and `pending` states to only match to the "end" of the NavLink's `to` path. If the URL is longer than `to`, it will no longer be considered active.
 
-| Link                          | URL          | isActive |
-| ----------------------------- | ------------ | -------- |
-| `<NavLink to="/tasks" />`     | `/tasks`     | true     |
-| `<NavLink to="/tasks" />`     | `/tasks/123` | true     |
-| `<NavLink to="/tasks" end />` | `/tasks`     | true     |
-| `<NavLink to="/tasks" end />` | `/tasks/123` | false    |
+| Link                           | Current URL  | isActive |
+| ------------------------------ | ------------ | -------- |
+| `<NavLink to="/tasks" />`      | `/tasks`     | true     |
+| `<NavLink to="/tasks" />`      | `/tasks/123` | true     |
+| `<NavLink to="/tasks" end />`  | `/tasks`     | true     |
+| `<NavLink to="/tasks" end />`  | `/tasks/123` | false    |
+| `<NavLink to="/tasks/" end />` | `/tasks`     | false    |
+| `<NavLink to="/tasks/" end />` | `/tasks/`    | true     |
 
 **A note on links to the root route**
 

diff --git a/packages/react-router-dom/__tests__/nav-link-active-test.tsx b/packages/react-router-dom/__tests__/nav-link-active-test.tsx
@@ -106,6 +106,26 @@ describe("NavLink", () => {
       expect(anchor.props.className).toMatch("active");
     });
 
+    it("when the current URL has a trailing slash", () => {
+      let renderer: TestRenderer.ReactTestRenderer;
+      TestRenderer.act(() => {
+        renderer = TestRenderer.create(
+          <MemoryRouter initialEntries={["/home/"]}>
+            <Routes>
+              <Route
+                path="/home"
+                element={<NavLink to="/home/">Home</NavLink>}
+              />
+            </Routes>
+          </MemoryRouter>
+        );
+      });
+
+      let anchor = renderer.root.findByType("a");
+
+      expect(anchor.props.className).toMatch("active");
+    });
+
     it("applies its className correctly when provided as a function", () => {
       let renderer: TestRenderer.ReactTestRenderer;
       TestRenderer.act(() => {
@@ -433,6 +453,32 @@ describe("NavLink", () => {
       expect(anchor.props.className).toMatch("active");
     });
 
+    it("In case of trailing slash at the end of link", () => {
+      let renderer: TestRenderer.ReactTestRenderer;
+      TestRenderer.act(() => {
+        renderer = TestRenderer.create(
+          <MemoryRouter initialEntries={["/home/child"]}>
+            <Routes>
+              <Route
+                path="home"
+                element={
+                  <div>
+                    <NavLink to="/home/">Home</NavLink>
+                    <Outlet />
+                  </div>
+                }
+              >
+                <Route path="child" element={<div>Child</div>} />
+              </Route>
+            </Routes>
+          </MemoryRouter>
+        );
+      });
+
+      let anchor = renderer.root.findByType("a");
+      expect(anchor.props.className).toMatch("active");
+    });
+
     describe("when end=true", () => {
       it("does not apply the default 'active' className to the underlying <a>", () => {
         let renderer: TestRenderer.ReactTestRenderer;
@@ -462,6 +508,68 @@ describe("NavLink", () => {
 
         expect(anchor.props.className).not.toMatch("active");
       });
+
+      it("Handles trailing slashes accordingly when the URL does not have a trailing slash", () => {
+        let renderer: TestRenderer.ReactTestRenderer;
+        TestRenderer.act(() => {
+          renderer = TestRenderer.create(
+            <MemoryRouter initialEntries={["/home"]}>
+              <Routes>
+                <Route
+                  path="home"
+                  element={
+                    <div>
+                      <NavLink to="/home" end>
+                        Home
+                      </NavLink>
+                      <NavLink to="/home/" end>
+                        Home
+                      </NavLink>
+                      <Outlet />
+                    </div>
+                  }
+                >
+                  <Route path="child" element={<div>Child</div>} />
+                </Route>
+              </Routes>
+            </MemoryRouter>
+          );
+        });
+
+        let anchors = renderer.root.findAllByType("a");
+        expect(anchors.map((a) => a.props.className)).toEqual(["active", ""]);
+      });
+
+      it("Handles trailing slashes accordingly when the URL has a trailing slash", () => {
+        let renderer: TestRenderer.ReactTestRenderer;
+        TestRenderer.act(() => {
+          renderer = TestRenderer.create(
+            <MemoryRouter initialEntries={["/home/"]}>
+              <Routes>
+                <Route
+                  path="home"
+                  element={
+                    <div>
+                      <NavLink to="/home" end>
+                        Home
+                      </NavLink>
+                      <NavLink to="/home/" end>
+                        Home
+                      </NavLink>
+                      <Outlet />
+                    </div>
+                  }
+                >
+                  <Route path="child" element={<div>Child</div>} />
+                </Route>
+              </Routes>
+            </MemoryRouter>
+          );
+        });
+
+        let anchors = renderer.root.findAllByType("a");
+        expect(anchors.map((a) => a.props.className)).toEqual(["", "active"]);
+      });
     });
   });
 

diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx
@@ -964,11 +964,20 @@ export const NavLink = React.forwardRef<HTMLAnchorElement, NavLinkProps>(
       toPathname = toPathname.toLowerCase();
     }
 
+    // If the `to` has a trailing slash, look at that exact spot.  Otherwise,
+    // we're looking for a slash _after_ what's in `to`.  For example:
+    //
+    // <NavLink to="/users"> and <NavLink to="/users/">
+    // both want to look for a / at index 6 to match URL `/users/matt`
+    const endSlashPosition =
+      toPathname !== "/" && toPathname.endsWith("/")
+        ? toPathname.length - 1
+        : toPathname.length;
     let isActive =
       locationPathname === toPathname ||
       (!end &&
         locationPathname.startsWith(toPathname) &&
-        locationPathname.charAt(toPathname.length) === "/");
+        locationPathname.charAt(endSlashPosition) === "/");
 
     let isPending =
       nextLocationPathname != null &&