diff --git a/airflow-core/src/airflow/ui/src/layouts/Nav/AdminButton.tsx b/airflow-core/src/airflow/ui/src/layouts/Nav/AdminButton.tsx index 427d994c55aa7..7037b3aaf1e61 100644 --- a/airflow-core/src/airflow/ui/src/layouts/Nav/AdminButton.tsx +++ b/airflow-core/src/airflow/ui/src/layouts/Nav/AdminButton.tsx @@ -62,15 +62,14 @@ export const AdminButton = ({ readonly externalViews: Array; }) => { const { t: translate } = useTranslation("common"); - const menuItems = links - .filter(({ title }) => authorizedMenuItems.includes(title as MenuItem)) - .map((link) => ( - - - {translate(`admin.${link.title}`)} - - - )); + const authorizedLinks = links.filter(({ title }) => authorizedMenuItems.includes(title as MenuItem)); + const menuItems = authorizedLinks.map((link) => ( + + + {translate(`admin.${link.title}`)} + + + )); if (!menuItems.length && !externalViews.length) { return undefined; @@ -79,7 +78,11 @@ export const AdminButton = ({ return ( - + href)} + /> {menuItems} diff --git a/airflow-core/src/airflow/ui/src/layouts/Nav/BrowseButton.tsx b/airflow-core/src/airflow/ui/src/layouts/Nav/BrowseButton.tsx index d95f9e0f31815..9ebb236ab0781 100644 --- a/airflow-core/src/airflow/ui/src/layouts/Nav/BrowseButton.tsx +++ b/airflow-core/src/airflow/ui/src/layouts/Nav/BrowseButton.tsx @@ -63,15 +63,14 @@ export const BrowseButton = ({ readonly externalViews: Array; }) => { const { t: translate } = useTranslation("common"); - const menuItems = links - .filter(({ title }) => authorizedMenuItems.includes(title as MenuItem)) - .map((link) => ( - - - {translate(`browse.${link.key}`)} - - - )); + const authorizedLinks = links.filter(({ title }) => authorizedMenuItems.includes(title as MenuItem)); + const menuItems = authorizedLinks.map((link) => ( + + + {translate(`browse.${link.key}`)} + + + )); if (!menuItems.length && !externalViews.length) { return undefined; @@ -80,7 +79,11 @@ export const BrowseButton = ({ return ( - + href)} + /> {menuItems} diff --git a/airflow-core/src/airflow/ui/src/layouts/Nav/Nav.tsx b/airflow-core/src/airflow/ui/src/layouts/Nav/Nav.tsx index 4e4e9e39b1728..39924dc18d06a 100644 --- a/airflow-core/src/airflow/ui/src/layouts/Nav/Nav.tsx +++ b/airflow-core/src/airflow/ui/src/layouts/Nav/Nav.tsx @@ -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" /> diff --git a/airflow-core/src/airflow/ui/src/layouts/Nav/NavButton.test.tsx b/airflow-core/src/airflow/ui/src/layouts/Nav/NavButton.test.tsx new file mode 100644 index 0000000000000..6671f4a109ad3 --- /dev/null +++ b/airflow-core/src/airflow/ui/src/layouts/Nav/NavButton.test.tsx @@ -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) => ( + + {children} + + ); + + return wrapper; +}; + +describe("NavButton", () => { + describe("single `to`", () => { + it("renders as a link to that destination", () => { + render(, { wrapper: wrapperAt("/") }); + + expect(screen.getByRole("link", { name: "Dags" })).toHaveAttribute("href", "/dags"); + }); + + it("is active when the current route matches", () => { + render(, { wrapper: wrapperAt("/dags") }); + + expect(screen.getByRole("link", { name: "Dags" })).toHaveAttribute("aria-current", "page"); + }); + + it("is active on a nested route under the destination", () => { + render(, { + 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(, { 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(, { + 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(, { + 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(, { + 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(, { + 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(, { + wrapper: wrapperAt("/assets"), + }); + + expect(screen.getByRole("link", { name: "Dags" })).not.toHaveAttribute("aria-current"); + }); + }); +}); diff --git a/airflow-core/src/airflow/ui/src/layouts/Nav/NavButton.tsx b/airflow-core/src/airflow/ui/src/layouts/Nav/NavButton.tsx index f943efc32aa1d..648ee43ce62b0 100644 --- a/airflow-core/src/airflow/ui/src/layouts/Nav/NavButton.tsx +++ b/airflow-core/src/airflow/ui/src/layouts/Nav/NavButton.tsx @@ -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 = []; const commonLabelProps: BoxProps = { fontSize: "2xs", @@ -33,21 +35,29 @@ const commonLabelProps: BoxProps = { type NavButtonProps = { readonly icon: ForwardRefExoticComponent> | 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; 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; } & 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 @@ -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", @@ -92,7 +103,7 @@ export const NavButton = ({ icon, isExternal = false, pluginIcon, title, to, ... ...rest, }; - if (to === undefined) { + if (to === undefined || Array.isArray(to)) { return (