Skip to content

Commit f3edf4d

Browse files
authored
Allow multiple routes to show active NavButtons (#69884)
* Allow multiple routes to show active NavButtons * clean up securitybutton
1 parent ae756a9 commit f3edf4d

7 files changed

Lines changed: 189 additions & 46 deletions

File tree

airflow-core/src/airflow/ui/src/layouts/Nav/AdminButton.tsx

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -62,15 +62,14 @@ export const AdminButton = ({
6262
readonly externalViews: Array<NavItemResponse>;
6363
}) => {
6464
const { t: translate } = useTranslation("common");
65-
const menuItems = links
66-
.filter(({ title }) => authorizedMenuItems.includes(title as MenuItem))
67-
.map((link) => (
68-
<Menu.Item asChild key={link.title} value={link.title}>
69-
<RouterLink aria-label={translate(`admin.${link.title}`)} to={link.href}>
70-
{translate(`admin.${link.title}`)}
71-
</RouterLink>
72-
</Menu.Item>
73-
));
65+
const authorizedLinks = links.filter(({ title }) => authorizedMenuItems.includes(title as MenuItem));
66+
const menuItems = authorizedLinks.map((link) => (
67+
<Menu.Item asChild key={link.title} value={link.title}>
68+
<RouterLink aria-label={translate(`admin.${link.title}`)} to={link.href}>
69+
{translate(`admin.${link.title}`)}
70+
</RouterLink>
71+
</Menu.Item>
72+
));
7473

7574
if (!menuItems.length && !externalViews.length) {
7675
return undefined;
@@ -79,7 +78,11 @@ export const AdminButton = ({
7978
return (
8079
<Menu.Root positioning={{ placement: "right" }}>
8180
<Menu.Trigger asChild>
82-
<NavButton icon={FiSettings} title={translate("nav.admin")} />
81+
<NavButton
82+
icon={FiSettings}
83+
title={translate("nav.admin")}
84+
to={authorizedLinks.map(({ href }) => href)}
85+
/>
8386
</Menu.Trigger>
8487
<Menu.Content>
8588
{menuItems}

airflow-core/src/airflow/ui/src/layouts/Nav/BrowseButton.tsx

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -63,15 +63,14 @@ export const BrowseButton = ({
6363
readonly externalViews: Array<NavItemResponse>;
6464
}) => {
6565
const { t: translate } = useTranslation("common");
66-
const menuItems = links
67-
.filter(({ title }) => authorizedMenuItems.includes(title as MenuItem))
68-
.map((link) => (
69-
<Menu.Item asChild key={link.key} value={translate(`browse.${link.key}`)}>
70-
<RouterLink aria-label={translate(`browse.${link.key}`)} to={link.href}>
71-
{translate(`browse.${link.key}`)}
72-
</RouterLink>
73-
</Menu.Item>
74-
));
66+
const authorizedLinks = links.filter(({ title }) => authorizedMenuItems.includes(title as MenuItem));
67+
const menuItems = authorizedLinks.map((link) => (
68+
<Menu.Item asChild key={link.key} value={translate(`browse.${link.key}`)}>
69+
<RouterLink aria-label={translate(`browse.${link.key}`)} to={link.href}>
70+
{translate(`browse.${link.key}`)}
71+
</RouterLink>
72+
</Menu.Item>
73+
));
7574

7675
if (!menuItems.length && !externalViews.length) {
7776
return undefined;
@@ -80,7 +79,11 @@ export const BrowseButton = ({
8079
return (
8180
<Menu.Root positioning={{ placement: "right" }}>
8281
<Menu.Trigger asChild>
83-
<NavButton icon={FiGlobe} title={translate("nav.browse")} />
82+
<NavButton
83+
icon={FiGlobe}
84+
title={translate("nav.browse")}
85+
to={authorizedLinks.map(({ href }) => href)}
86+
/>
8487
</Menu.Trigger>
8588
<Menu.Content>
8689
{menuItems}

airflow-core/src/airflow/ui/src/layouts/Nav/Nav.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,7 @@ export const Nav = () => {
176176
data-testid="nav-dags-link"
177177
disabled={!authLinks?.authorized_menu_items.includes("Dags")}
178178
icon={DagIcon}
179+
matchPaths={["dag_runs", "task_instances"]}
179180
title={translate("nav.dags")}
180181
to="dags"
181182
/>
Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
/*!
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
import "@testing-library/jest-dom";
20+
import { render, screen } from "@testing-library/react";
21+
import type { PropsWithChildren } from "react";
22+
import { FiHome } from "react-icons/fi";
23+
import { MemoryRouter } from "react-router-dom";
24+
import { describe, expect, it } from "vitest";
25+
26+
import { BaseWrapper } from "src/utils/Wrapper";
27+
28+
import { NavButton } from "./NavButton";
29+
30+
const wrapperAt = (path: string) => {
31+
const wrapper = ({ children }: PropsWithChildren) => (
32+
<BaseWrapper>
33+
<MemoryRouter initialEntries={[path]}>{children}</MemoryRouter>
34+
</BaseWrapper>
35+
);
36+
37+
return wrapper;
38+
};
39+
40+
describe("NavButton", () => {
41+
describe("single `to`", () => {
42+
it("renders as a link to that destination", () => {
43+
render(<NavButton icon={FiHome} title="Dags" to="dags" />, { wrapper: wrapperAt("/") });
44+
45+
expect(screen.getByRole("link", { name: "Dags" })).toHaveAttribute("href", "/dags");
46+
});
47+
48+
it("is active when the current route matches", () => {
49+
render(<NavButton icon={FiHome} title="Dags" to="dags" />, { wrapper: wrapperAt("/dags") });
50+
51+
expect(screen.getByRole("link", { name: "Dags" })).toHaveAttribute("aria-current", "page");
52+
});
53+
54+
it("is active on a nested route under the destination", () => {
55+
render(<NavButton icon={FiHome} title="Dags" to="dags" />, {
56+
wrapper: wrapperAt("/dags/my_dag/runs"),
57+
});
58+
59+
expect(screen.getByRole("link", { name: "Dags" })).toHaveAttribute("aria-current", "page");
60+
});
61+
62+
it("is not active on an unrelated route", () => {
63+
render(<NavButton icon={FiHome} title="Dags" to="dags" />, { wrapper: wrapperAt("/assets") });
64+
65+
expect(screen.getByRole("link", { name: "Dags" })).not.toHaveAttribute("aria-current");
66+
});
67+
});
68+
69+
describe("multiple `to`", () => {
70+
it("renders as a plain button, not a link", () => {
71+
render(<NavButton icon={FiHome} title="Browse" to={["events", "jobs"]} />, {
72+
wrapper: wrapperAt("/"),
73+
});
74+
75+
const button = screen.getByRole("button", { name: "Browse" });
76+
77+
expect(button).not.toHaveAttribute("href");
78+
});
79+
80+
it("is active when the current route matches any of the destinations", () => {
81+
render(<NavButton icon={FiHome} title="Browse" to={["events", "jobs"]} />, {
82+
wrapper: wrapperAt("/jobs"),
83+
});
84+
85+
expect(screen.getByRole("button", { name: "Browse" })).toHaveAttribute("aria-current", "page");
86+
});
87+
88+
it("is not active when the current route matches none of the destinations", () => {
89+
render(<NavButton icon={FiHome} title="Browse" to={["events", "jobs"]} />, {
90+
wrapper: wrapperAt("/xcoms"),
91+
});
92+
93+
expect(screen.getByRole("button", { name: "Browse" })).not.toHaveAttribute("aria-current");
94+
});
95+
});
96+
97+
describe("matchPaths", () => {
98+
it("is active on an extra match path even though `to` points elsewhere", () => {
99+
render(<NavButton icon={FiHome} matchPaths={["dag_runs", "task_instances"]} title="Dags" to="dags" />, {
100+
wrapper: wrapperAt("/dag_runs"),
101+
});
102+
103+
expect(screen.getByRole("link", { name: "Dags" })).toHaveAttribute("aria-current", "page");
104+
});
105+
106+
it("is not active on a route outside both `to` and matchPaths", () => {
107+
render(<NavButton icon={FiHome} matchPaths={["dag_runs", "task_instances"]} title="Dags" to="dags" />, {
108+
wrapper: wrapperAt("/assets"),
109+
});
110+
111+
expect(screen.getByRole("link", { name: "Dags" })).not.toHaveAttribute("aria-current");
112+
});
113+
});
114+
});

airflow-core/src/airflow/ui/src/layouts/Nav/NavButton.tsx

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@
1919
import { Box, type BoxProps, Button, Icon, type IconProps, Link, type ButtonProps } from "@chakra-ui/react";
2020
import type { ReactNode, ForwardRefExoticComponent, RefAttributes } from "react";
2121
import type { IconType } from "react-icons";
22-
import { Link as RouterLink, useMatch } from "react-router-dom";
22+
import { Link as RouterLink, matchPath, useLocation } from "react-router-dom";
23+
24+
const noMatchPaths: Array<string> = [];
2325

2426
const commonLabelProps: BoxProps = {
2527
fontSize: "2xs",
@@ -33,21 +35,29 @@ const commonLabelProps: BoxProps = {
3335
type NavButtonProps = {
3436
readonly icon: ForwardRefExoticComponent<IconProps & RefAttributes<SVGSVGElement>> | IconType;
3537
readonly isExternal?: boolean;
38+
// Extra routes that should also mark this button active, on top of `to` (e.g. the Dags button
39+
// should also highlight for the standalone dag runs and task instances routes).
40+
readonly matchPaths?: Array<string>;
3641
readonly pluginIcon?: ReactNode;
3742
readonly title: string;
38-
readonly to?: string;
43+
// A single destination renders the button as a link; an array only affects isActive matching
44+
// (used for buttons like menu triggers that should highlight for any of several routes).
45+
readonly to?: Array<string> | string;
3946
} & ButtonProps;
4047

41-
export const NavButton = ({ icon, isExternal = false, pluginIcon, title, to, ...rest }: NavButtonProps) => {
42-
// Use useMatch to determine if the current route matches the button's destination
43-
// This provides the same functionality as NavLink's isActive prop
44-
// Only applies to buttons with a to prop (but needs to be before any return statements)
45-
const match = useMatch({
46-
end: to === "/", // Only exact match for root path
47-
path: to ?? "",
48-
});
49-
// Only applies to buttons with a to prop
50-
const isActive = Boolean(to) ? Boolean(match) : false;
48+
export const NavButton = ({
49+
icon,
50+
isExternal = false,
51+
matchPaths = noMatchPaths,
52+
pluginIcon,
53+
title,
54+
to,
55+
...rest
56+
}: NavButtonProps) => {
57+
const { pathname } = useLocation();
58+
59+
const activePaths = [...(to === undefined ? [] : Array.isArray(to) ? to : [to]), ...matchPaths];
60+
const isActive = activePaths.some((path) => matchPath({ end: path === "/", path }, pathname) !== null);
5161

5262
const commonButtonProps: ButtonProps = {
5363
_expanded: isActive
@@ -72,6 +82,7 @@ export const NavButton = ({ icon, isExternal = false, pluginIcon, title, to, ...
7282
color: "fg",
7383
},
7484
alignItems: "center",
85+
"aria-current": isActive ? "page" : undefined,
7586
"aria-label": title,
7687
bg: isActive ? "brand.solid" : undefined,
7788
borderRadius: "md",
@@ -92,7 +103,7 @@ export const NavButton = ({ icon, isExternal = false, pluginIcon, title, to, ...
92103
...rest,
93104
};
94105

95-
if (to === undefined) {
106+
if (to === undefined || Array.isArray(to)) {
96107
return (
97108
<Button {...commonButtonProps}>
98109
{pluginIcon ?? <Icon as={icon} boxSize={5} />}

airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenus.tsx

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,11 @@ export const PluginMenus = ({ navItems }: { readonly navItems: Array<NavItemResp
5353
// A single remaining item is promoted to the toolbar to avoid a one-item submenu.
5454
const showRemainingInMenu = remainingItems.length >= 2;
5555

56+
// Only internal routes (backed by url_route) can match the current pathname; external views never do.
57+
const remainingPaths = remainingItems
58+
.filter((navItem) => navItem.url_route !== undefined && navItem.url_route !== null)
59+
.map((navItem) => `plugin/${navItem.url_route}`);
60+
5661
return (
5762
<>
5863
{promotedItems.map((navItem) => (
@@ -61,7 +66,7 @@ export const PluginMenus = ({ navItems }: { readonly navItems: Array<NavItemResp
6166
{showRemainingInMenu ? (
6267
<Menu.Root positioning={{ placement: "right" }}>
6368
<Menu.Trigger>
64-
<NavButton as={Box} icon={LuPlug} title={translate("nav.plugins")} />
69+
<NavButton as={Box} icon={LuPlug} title={translate("nav.plugins")} to={remainingPaths} />
6570
</Menu.Trigger>
6671
<Menu.Content>
6772
{remainingButtons.map((navItem) => (

airflow-core/src/airflow/ui/src/layouts/Nav/SecurityButton.tsx

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,23 +33,29 @@ export const SecurityButton = () => {
3333
return undefined;
3434
}
3535

36+
const securityItems = authLinks.extra_menu_items.map(({ text }) => {
37+
const securityKey = text.toLowerCase().replace(" ", "-");
38+
39+
return { path: `security/${securityKey}`, securityKey, text };
40+
});
41+
3642
return (
3743
<Menu.Root positioning={{ placement: "right" }}>
3844
<Menu.Trigger asChild>
39-
<NavButton icon={FiLock} title={translate("nav.security")} />
45+
<NavButton
46+
icon={FiLock}
47+
title={translate("nav.security")}
48+
to={securityItems.map(({ path }) => path)}
49+
/>
4050
</Menu.Trigger>
4151
<Menu.Content>
42-
{authLinks.extra_menu_items.map(({ text }) => {
43-
const securityKey = text.toLowerCase().replace(" ", "-");
44-
45-
return (
46-
<Menu.Item asChild key={text} value={text}>
47-
<Link aria-label={text} to={`security/${securityKey}`}>
48-
{translate(`security.${securityKey}`)}
49-
</Link>
50-
</Menu.Item>
51-
);
52-
})}
52+
{securityItems.map(({ path, securityKey, text }) => (
53+
<Menu.Item asChild key={text} value={text}>
54+
<Link aria-label={text} to={path}>
55+
{translate(`security.${securityKey}`)}
56+
</Link>
57+
</Menu.Item>
58+
))}
5359
</Menu.Content>
5460
</Menu.Root>
5561
);

0 commit comments

Comments
 (0)