Skip to content
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
23 changes: 13 additions & 10 deletions airflow-core/src/airflow/ui/src/layouts/Nav/AdminButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,14 @@ export const AdminButton = ({
readonly externalViews: Array<NavItemResponse>;
}) => {
const { t: translate } = useTranslation("common");
const menuItems = links
.filter(({ title }) => authorizedMenuItems.includes(title as MenuItem))
.map((link) => (
<Menu.Item asChild key={link.title} value={link.title}>
<RouterLink aria-label={translate(`admin.${link.title}`)} to={link.href}>
{translate(`admin.${link.title}`)}
</RouterLink>
</Menu.Item>
));
const authorizedLinks = links.filter(({ title }) => authorizedMenuItems.includes(title as MenuItem));
const menuItems = authorizedLinks.map((link) => (
<Menu.Item asChild key={link.title} value={link.title}>
<RouterLink aria-label={translate(`admin.${link.title}`)} to={link.href}>
{translate(`admin.${link.title}`)}
</RouterLink>
</Menu.Item>
));

if (!menuItems.length && !externalViews.length) {
return undefined;
Expand All @@ -79,7 +78,11 @@ export const AdminButton = ({
return (
<Menu.Root positioning={{ placement: "right" }}>
<Menu.Trigger asChild>
<NavButton icon={FiSettings} title={translate("nav.admin")} />
<NavButton
icon={FiSettings}
title={translate("nav.admin")}
to={authorizedLinks.map(({ href }) => href)}
/>
</Menu.Trigger>
<Menu.Content>
{menuItems}
Expand Down
23 changes: 13 additions & 10 deletions airflow-core/src/airflow/ui/src/layouts/Nav/BrowseButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,14 @@ export const BrowseButton = ({
readonly externalViews: Array<NavItemResponse>;
}) => {
const { t: translate } = useTranslation("common");
const menuItems = links
.filter(({ title }) => authorizedMenuItems.includes(title as MenuItem))
.map((link) => (
<Menu.Item asChild key={link.key} value={translate(`browse.${link.key}`)}>
<RouterLink aria-label={translate(`browse.${link.key}`)} to={link.href}>
{translate(`browse.${link.key}`)}
</RouterLink>
</Menu.Item>
));
const authorizedLinks = links.filter(({ title }) => authorizedMenuItems.includes(title as MenuItem));
const menuItems = authorizedLinks.map((link) => (
<Menu.Item asChild key={link.key} value={translate(`browse.${link.key}`)}>
<RouterLink aria-label={translate(`browse.${link.key}`)} to={link.href}>
{translate(`browse.${link.key}`)}
</RouterLink>
</Menu.Item>
));

if (!menuItems.length && !externalViews.length) {
return undefined;
Expand All @@ -80,7 +79,11 @@ export const BrowseButton = ({
return (
<Menu.Root positioning={{ placement: "right" }}>
<Menu.Trigger asChild>
<NavButton icon={FiGlobe} title={translate("nav.browse")} />
<NavButton
icon={FiGlobe}
title={translate("nav.browse")}
to={authorizedLinks.map(({ href }) => href)}
/>
</Menu.Trigger>
<Menu.Content>
{menuItems}
Expand Down
1 change: 1 addition & 0 deletions airflow-core/src/airflow/ui/src/layouts/Nav/Nav.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ export const Nav = () => {
data-testid="nav-dags-link"
disabled={!authLinks?.authorized_menu_items.includes("Dags")}
icon={DagIcon}
matchPaths={["dag_runs", "task_instances"]}
title={translate("nav.dags")}
to="dags"
/>
Expand Down
114 changes: 114 additions & 0 deletions airflow-core/src/airflow/ui/src/layouts/Nav/NavButton.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/*!
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import "@testing-library/jest-dom";
import { render, screen } from "@testing-library/react";
import type { PropsWithChildren } from "react";
import { FiHome } from "react-icons/fi";
import { MemoryRouter } from "react-router-dom";
import { describe, expect, it } from "vitest";

import { BaseWrapper } from "src/utils/Wrapper";

import { NavButton } from "./NavButton";

const wrapperAt = (path: string) => {
const wrapper = ({ children }: PropsWithChildren) => (
<BaseWrapper>
<MemoryRouter initialEntries={[path]}>{children}</MemoryRouter>
</BaseWrapper>
);

return wrapper;
};

describe("NavButton", () => {
describe("single `to`", () => {
it("renders as a link to that destination", () => {
render(<NavButton icon={FiHome} title="Dags" to="dags" />, { wrapper: wrapperAt("/") });

expect(screen.getByRole("link", { name: "Dags" })).toHaveAttribute("href", "/dags");
});

it("is active when the current route matches", () => {
render(<NavButton icon={FiHome} title="Dags" to="dags" />, { wrapper: wrapperAt("/dags") });

expect(screen.getByRole("link", { name: "Dags" })).toHaveAttribute("aria-current", "page");
});

it("is active on a nested route under the destination", () => {
render(<NavButton icon={FiHome} title="Dags" to="dags" />, {
wrapper: wrapperAt("/dags/my_dag/runs"),
});

expect(screen.getByRole("link", { name: "Dags" })).toHaveAttribute("aria-current", "page");
});

it("is not active on an unrelated route", () => {
render(<NavButton icon={FiHome} title="Dags" to="dags" />, { wrapper: wrapperAt("/assets") });

expect(screen.getByRole("link", { name: "Dags" })).not.toHaveAttribute("aria-current");
});
});

describe("multiple `to`", () => {
it("renders as a plain button, not a link", () => {
render(<NavButton icon={FiHome} title="Browse" to={["events", "jobs"]} />, {
wrapper: wrapperAt("/"),
});

const button = screen.getByRole("button", { name: "Browse" });

expect(button).not.toHaveAttribute("href");
});

it("is active when the current route matches any of the destinations", () => {
render(<NavButton icon={FiHome} title="Browse" to={["events", "jobs"]} />, {
wrapper: wrapperAt("/jobs"),
});

expect(screen.getByRole("button", { name: "Browse" })).toHaveAttribute("aria-current", "page");
});

it("is not active when the current route matches none of the destinations", () => {
render(<NavButton icon={FiHome} title="Browse" to={["events", "jobs"]} />, {
wrapper: wrapperAt("/xcoms"),
});

expect(screen.getByRole("button", { name: "Browse" })).not.toHaveAttribute("aria-current");
});
});

describe("matchPaths", () => {
it("is active on an extra match path even though `to` points elsewhere", () => {
render(<NavButton icon={FiHome} matchPaths={["dag_runs", "task_instances"]} title="Dags" to="dags" />, {
wrapper: wrapperAt("/dag_runs"),
});

expect(screen.getByRole("link", { name: "Dags" })).toHaveAttribute("aria-current", "page");
});

it("is not active on a route outside both `to` and matchPaths", () => {
render(<NavButton icon={FiHome} matchPaths={["dag_runs", "task_instances"]} title="Dags" to="dags" />, {
wrapper: wrapperAt("/assets"),
});

expect(screen.getByRole("link", { name: "Dags" })).not.toHaveAttribute("aria-current");
});
});
});
37 changes: 24 additions & 13 deletions airflow-core/src/airflow/ui/src/layouts/Nav/NavButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
import { Box, type BoxProps, Button, Icon, type IconProps, Link, type ButtonProps } from "@chakra-ui/react";
import type { ReactNode, ForwardRefExoticComponent, RefAttributes } from "react";
import type { IconType } from "react-icons";
import { Link as RouterLink, useMatch } from "react-router-dom";
import { Link as RouterLink, matchPath, useLocation } from "react-router-dom";

const noMatchPaths: Array<string> = [];

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

export const NavButton = ({ icon, isExternal = false, pluginIcon, title, to, ...rest }: NavButtonProps) => {
// Use useMatch to determine if the current route matches the button's destination
// This provides the same functionality as NavLink's isActive prop
// Only applies to buttons with a to prop (but needs to be before any return statements)
const match = useMatch({
end: to === "/", // Only exact match for root path
path: to ?? "",
});
// Only applies to buttons with a to prop
const isActive = Boolean(to) ? Boolean(match) : false;
export const NavButton = ({
icon,
isExternal = false,
matchPaths = noMatchPaths,
pluginIcon,
title,
to,
...rest
}: NavButtonProps) => {
const { pathname } = useLocation();

const activePaths = [...(to === undefined ? [] : Array.isArray(to) ? to : [to]), ...matchPaths];
const isActive = activePaths.some((path) => matchPath({ end: path === "/", path }, pathname) !== null);

const commonButtonProps: ButtonProps = {
_expanded: isActive
Expand All @@ -72,6 +82,7 @@ export const NavButton = ({ icon, isExternal = false, pluginIcon, title, to, ...
color: "fg",
},
alignItems: "center",
"aria-current": isActive ? "page" : undefined,
"aria-label": title,
bg: isActive ? "brand.solid" : undefined,
borderRadius: "md",
Expand All @@ -92,7 +103,7 @@ export const NavButton = ({ icon, isExternal = false, pluginIcon, title, to, ...
...rest,
};

if (to === undefined) {
if (to === undefined || Array.isArray(to)) {
return (
<Button {...commonButtonProps}>
{pluginIcon ?? <Icon as={icon} boxSize={5} />}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ export const PluginMenus = ({ navItems }: { readonly navItems: Array<NavItemResp
// A single remaining item is promoted to the toolbar to avoid a one-item submenu.
const showRemainingInMenu = remainingItems.length >= 2;

// Only internal routes (backed by url_route) can match the current pathname; external views never do.
const remainingPaths = remainingItems
.filter((navItem) => navItem.url_route !== undefined && navItem.url_route !== null)
.map((navItem) => `plugin/${navItem.url_route}`);

return (
<>
{promotedItems.map((navItem) => (
Expand All @@ -61,7 +66,7 @@ export const PluginMenus = ({ navItems }: { readonly navItems: Array<NavItemResp
{showRemainingInMenu ? (
<Menu.Root positioning={{ placement: "right" }}>
<Menu.Trigger>
<NavButton as={Box} icon={LuPlug} title={translate("nav.plugins")} />
<NavButton as={Box} icon={LuPlug} title={translate("nav.plugins")} to={remainingPaths} />
</Menu.Trigger>
<Menu.Content>
{remainingButtons.map((navItem) => (
Expand Down
30 changes: 18 additions & 12 deletions airflow-core/src/airflow/ui/src/layouts/Nav/SecurityButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,23 +33,29 @@ export const SecurityButton = () => {
return undefined;
}

const securityItems = authLinks.extra_menu_items.map(({ text }) => {
const securityKey = text.toLowerCase().replace(" ", "-");

return { path: `security/${securityKey}`, securityKey, text };
});

return (
<Menu.Root positioning={{ placement: "right" }}>
<Menu.Trigger asChild>
<NavButton icon={FiLock} title={translate("nav.security")} />
<NavButton
icon={FiLock}
title={translate("nav.security")}
to={securityItems.map(({ path }) => path)}
/>
</Menu.Trigger>
<Menu.Content>
{authLinks.extra_menu_items.map(({ text }) => {
const securityKey = text.toLowerCase().replace(" ", "-");

return (
<Menu.Item asChild key={text} value={text}>
<Link aria-label={text} to={`security/${securityKey}`}>
{translate(`security.${securityKey}`)}
</Link>
</Menu.Item>
);
})}
{securityItems.map(({ path, securityKey, text }) => (
<Menu.Item asChild key={text} value={text}>
<Link aria-label={text} to={path}>
{translate(`security.${securityKey}`)}
</Link>
</Menu.Item>
))}
</Menu.Content>
</Menu.Root>
);
Expand Down
Loading