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

Fixed NavLink Issue #10734

Merged
merged 12 commits into from
Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/itchy-items-hang.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-router-dom": patch
---

Fix `NavLink` `active` logic when `to` location has a trailing slash
1 change: 1 addition & 0 deletions contributors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
- bbrowning918
- BDomzalski
- bhbs
- bilalk711
- bobziroll
- BrianT1414
- brockross
Expand Down
14 changes: 8 additions & 6 deletions docs/components/nav-link.md
Original file line number Diff line number Diff line change
Expand Up @@ -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**

Expand Down
108 changes: 108 additions & 0 deletions packages/react-router-dom/__tests__/nav-link-active-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -433,6 +453,32 @@ describe("NavLink", () => {
expect(anchor.props.className).toMatch("active");
});

bilalk711 marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand Down Expand Up @@ -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"]);
});
});
});

Expand Down
11 changes: 10 additions & 1 deletion packages/react-router-dom/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down