From e81275a91c0fe28585766ddf7c93b4b9f5f3af55 Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Thu, 2 Apr 2026 03:45:20 +1100 Subject: [PATCH] feat: cleanup `` component (#23839) This refactors `` into two clearer patterns: link tabs for route navigation and Radix tabs for stateful tab panels. That gives us proper accessibility semantics where we need them without overloading simple navigation tabs. As part of that split, this updates several consumers, adds coverage for both variants, and cleans up some nearby styling. - introduce Radix-backed tabs primitives for tabbed content - move router-based tabs to `LinkTabs` - update notifications, IdP sync, and workspace build pages to use semantic tabs - preserve route navigation tabs for groups and templates - add stories/tests for both tab implementations - simplify related layout and styling in touched components --- site/package.json | 1 + site/pnpm-lock.yaml | 32 +++ .../PageHeader/FullWidthPageHeader.tsx | 60 +----- site/src/components/Tabs/Tabs.stories.tsx | 57 +++++- site/src/components/Tabs/Tabs.test.tsx | 49 ++++- site/src/components/Tabs/Tabs.tsx | 111 +++++++++-- .../WorkspaceBuildLogs/WorkspaceBuildLogs.tsx | 20 +- .../NotificationsPage/NotificationsPage.tsx | 67 ++++--- site/src/pages/GroupsPage/GroupPage.tsx | 10 +- .../IdpSyncPage/IdpSyncPageView.tsx | 71 ++++--- .../src/pages/TemplatePage/TemplateLayout.tsx | 10 +- site/src/pages/WorkspaceBuildPage/Sidebar.tsx | 13 +- .../WorkspaceBuildPage.jest.tsx | 49 ++++- .../WorkspaceBuildPageView.tsx | 186 +++++++++--------- .../WorkspaceBuildLogsSection.tsx | 2 +- 15 files changed, 476 insertions(+), 262 deletions(-) diff --git a/site/package.json b/site/package.json index 95095e722b..aca8d8546b 100644 --- a/site/package.json +++ b/site/package.json @@ -77,6 +77,7 @@ "@radix-ui/react-slider": "1.3.6", "@radix-ui/react-slot": "1.2.4", "@radix-ui/react-switch": "1.2.6", + "@radix-ui/react-tabs": "1.1.13", "@radix-ui/react-tooltip": "1.2.8", "@tanstack/react-query-devtools": "5.77.0", "@xterm/addon-canvas": "0.7.0", diff --git a/site/pnpm-lock.yaml b/site/pnpm-lock.yaml index c21da6e74b..1a7ca03318 100644 --- a/site/pnpm-lock.yaml +++ b/site/pnpm-lock.yaml @@ -130,6 +130,9 @@ importers: '@radix-ui/react-switch': specifier: 1.2.6 version: 1.2.6(@types/react-dom@19.2.3(@types/react@19.2.7))(@types/react@19.2.7)(react-dom@19.2.2(react@19.2.2))(react@19.2.2) + '@radix-ui/react-tabs': + specifier: 1.1.13 + version: 1.1.13(@types/react-dom@19.2.3(@types/react@19.2.7))(@types/react@19.2.7)(react-dom@19.2.2(react@19.2.2))(react@19.2.2) '@radix-ui/react-tooltip': specifier: 1.2.8 version: 1.2.8(@types/react-dom@19.2.3(@types/react@19.2.7))(@types/react@19.2.7)(react-dom@19.2.2(react@19.2.2))(react@19.2.2) @@ -2134,6 +2137,19 @@ packages: '@types/react-dom': optional: true + '@radix-ui/react-tabs@1.1.13': + resolution: {integrity: sha512-7xdcatg7/U+7+Udyoj2zodtI9H/IIopqo+YOIcZOq1nJwXWBZ9p8xiu5llXlekDbZkca79a/fozEYQXIA4sW6A==, tarball: https://registry.npmjs.org/@radix-ui/react-tabs/-/react-tabs-1.1.13.tgz} + peerDependencies: + '@types/react': '*' + '@types/react-dom': '*' + react: ^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc + react-dom: ^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc + peerDependenciesMeta: + '@types/react': + optional: true + '@types/react-dom': + optional: true + '@radix-ui/react-tooltip@1.2.8': resolution: {integrity: sha512-tY7sVt1yL9ozIxvmbtN5qtmH2krXcBCfjEiCgKGLqunJHvgvZG2Pcl2oQ3kbcZARb1BGEHdkLzcYGO8ynVlieg==, tarball: https://registry.npmjs.org/@radix-ui/react-tooltip/-/react-tooltip-1.2.8.tgz} peerDependencies: @@ -9097,6 +9113,22 @@ snapshots: '@types/react': 19.2.7 '@types/react-dom': 19.2.3(@types/react@19.2.7) + '@radix-ui/react-tabs@1.1.13(@types/react-dom@19.2.3(@types/react@19.2.7))(@types/react@19.2.7)(react-dom@19.2.2(react@19.2.2))(react@19.2.2)': + dependencies: + '@radix-ui/primitive': 1.1.3 + '@radix-ui/react-context': 1.1.2(@types/react@19.2.7)(react@19.2.2) + '@radix-ui/react-direction': 1.1.1(@types/react@19.2.7)(react@19.2.2) + '@radix-ui/react-id': 1.1.1(@types/react@19.2.7)(react@19.2.2) + '@radix-ui/react-presence': 1.1.5(@types/react-dom@19.2.3(@types/react@19.2.7))(@types/react@19.2.7)(react-dom@19.2.2(react@19.2.2))(react@19.2.2) + '@radix-ui/react-primitive': 2.1.3(@types/react-dom@19.2.3(@types/react@19.2.7))(@types/react@19.2.7)(react-dom@19.2.2(react@19.2.2))(react@19.2.2) + '@radix-ui/react-roving-focus': 1.1.11(@types/react-dom@19.2.3(@types/react@19.2.7))(@types/react@19.2.7)(react-dom@19.2.2(react@19.2.2))(react@19.2.2) + '@radix-ui/react-use-controllable-state': 1.2.2(@types/react@19.2.7)(react@19.2.2) + react: 19.2.2 + react-dom: 19.2.2(react@19.2.2) + optionalDependencies: + '@types/react': 19.2.7 + '@types/react-dom': 19.2.3(@types/react@19.2.7) + '@radix-ui/react-tooltip@1.2.8(@types/react-dom@19.2.3(@types/react@19.2.7))(@types/react@19.2.7)(react-dom@19.2.2(react@19.2.2))(react@19.2.2)': dependencies: '@radix-ui/primitive': 1.1.3 diff --git a/site/src/components/PageHeader/FullWidthPageHeader.tsx b/site/src/components/PageHeader/FullWidthPageHeader.tsx index ac1ccfcaa4..4cb237d40d 100644 --- a/site/src/components/PageHeader/FullWidthPageHeader.tsx +++ b/site/src/components/PageHeader/FullWidthPageHeader.tsx @@ -1,5 +1,5 @@ -import { type CSSObject, useTheme } from "@emotion/react"; import type { FC, PropsWithChildren, ReactNode } from "react"; +import { cn } from "#/utils/cn"; interface FullWidthPageHeaderProps { children?: ReactNode; @@ -10,36 +10,15 @@ export const FullWidthPageHeader: FC = ({ children, sticky = true, }) => { - const theme = useTheme(); return (
{children}
@@ -47,19 +26,7 @@ export const FullWidthPageHeader: FC = ({ }; const _PageHeaderActions: FC = ({ children }) => { - const theme = useTheme(); - return ( -
- {children} -
- ); + return
{children}
; }; export const PageHeaderTitle: FC = ({ children }) => { @@ -67,16 +34,7 @@ export const PageHeaderTitle: FC = ({ children }) => { }; export const PageHeaderSubtitle: FC = ({ children }) => { - const theme = useTheme(); return ( - - {children} - + {children} ); }; diff --git a/site/src/components/Tabs/Tabs.stories.tsx b/site/src/components/Tabs/Tabs.stories.tsx index aa38e54776..9b7ba53d98 100644 --- a/site/src/components/Tabs/Tabs.stories.tsx +++ b/site/src/components/Tabs/Tabs.stories.tsx @@ -1,19 +1,27 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; -import { TabLink, Tabs, TabsList } from "./Tabs"; +import { + LinkTabs, + LinkTabsList, + TabLink, + Tabs, + TabsContent, + TabsList, + TabsTrigger, +} from "./Tabs"; -const meta: Meta = { +const meta: Meta = { title: "components/Tabs", - component: Tabs, + component: LinkTabs, }; export default meta; -type Story = StoryObj; +type Story = StoryObj; -export const Default: Story = { +export const LinkNavigation: Story = { args: { active: "tab-1", children: ( - + Tab 1 @@ -23,7 +31,42 @@ export const Default: Story = { Tab 3 - + ), }, + render: (args) => , +}; + +export const RadixInsideBox: StoryObj = { + render: () => ( + + + Alpha + Beta + + + Panel A + + + Panel B + + + ), +}; + +export const RadixOutsideBox: StoryObj = { + render: () => ( + + + Alpha + Beta + + + Panel A + + + Panel B + + + ), }; diff --git a/site/src/components/Tabs/Tabs.test.tsx b/site/src/components/Tabs/Tabs.test.tsx index b0bb195005..4f849c5c6e 100644 --- a/site/src/components/Tabs/Tabs.test.tsx +++ b/site/src/components/Tabs/Tabs.test.tsx @@ -1,33 +1,41 @@ import { render, screen } from "@testing-library/react"; import { MemoryRouter } from "react-router"; -import { TabLink, Tabs, TabsList } from "./Tabs"; +import { + LinkTabs, + LinkTabsList, + TabLink, + Tabs, + TabsContent, + TabsList, + TabsTrigger, +} from "./Tabs"; -const renderTabs = (active = "overview") => { +const renderLinkTabs = (active = "overview") => { render( - - + + Overview Settings - - + + , ); }; -describe("Tabs", () => { +describe("LinkTabs", () => { it("does not expose tablist semantics for link navigation", () => { - renderTabs(); + renderLinkTabs(); expect(screen.queryByRole("tablist")).not.toBeInTheDocument(); }); it("marks only the active tab link as the current page", () => { - renderTabs("overview"); + renderLinkTabs("overview"); expect(screen.getByRole("link", { name: "Overview" })).toHaveAttribute( "aria-current", @@ -38,3 +46,26 @@ describe("Tabs", () => { ); }); }); + +describe("Tabs (Radix)", () => { + it("exposes tablist semantics for keyboard navigation", () => { + render( + + + Alpha + Beta + + A + B + , + ); + + expect( + screen.getByRole("tablist", { name: "Example" }), + ).toBeInTheDocument(); + expect(screen.getByRole("tab", { name: "Alpha" })).toHaveAttribute( + "data-state", + "active", + ); + }); +}); diff --git a/site/src/components/Tabs/Tabs.tsx b/site/src/components/Tabs/Tabs.tsx index 4886f2c6ef..0da7b83579 100644 --- a/site/src/components/Tabs/Tabs.tsx +++ b/site/src/components/Tabs/Tabs.tsx @@ -1,4 +1,7 @@ +import * as TabsPrimitive from "@radix-ui/react-tabs"; +import { cva, type VariantProps } from "class-variance-authority"; import { + type ComponentProps, createContext, type FC, type HTMLAttributes, @@ -11,20 +14,101 @@ import { import { Link, type LinkProps } from "react-router"; import { cn } from "#/utils/cn"; -// Keeping this for now because of a workaround in WorkspaceBUildPageView +// --- Radix tabs (stateful panels) --- + +type TabsProps = ComponentProps; + +export const Tabs: FC = ({ ...props }) => { + return ; +}; + +const tabsListVariants = cva("flex flex-wrap items-center", { + variants: { + variant: { + insideBox: cn( + "border-solid border-x-0 border-y", + "[&_button[data-state=active]]:bg-surface-secondary", + "[&_button]:border-x [&_button]:border-y-0 [&_button]:border-solid", + "[&_button]:border-x-transparent [&_button[data-state=active]]:border-x-border", + "[&_button]:px-4", + "[&_button]:text-content-secondary", + "[&_button[data-state=active]]:text-content-primary", + ), + outsideBox: cn( + "border-solid border-0 border-b gap-6", + "[&_button]:text-content-secondary [&_button[data-state=active]]:text-content-primary", + "[&_button]:border-0 [&_button]:border-y [&_button]:border-solid", + "[&_button]:border-transparent [&_button[data-state=active]]:border-b-white", + "[&_button]:hover:text-content-primary", + "[&_button]:px-1", + ), + }, + }, + defaultVariants: { + variant: "outsideBox", + }, +}); +type TabsListProps = ComponentProps & + VariantProps; + +export const TabsList: FC = ({ + className, + variant, + ...props +}) => { + return ( + + ); +}; + +type TabsTriggerProps = ComponentProps; + +export const TabsTrigger: FC = ({ ...props }) => { + return ( + + ); +}; + +type TabsContentProps = ComponentProps; + +export const TabsContent: FC = ({ ...props }) => { + return ; +}; + +// --- Router link tabs (URL-driven navigation) --- + +// Keeping this for now because of a workaround in WorkspaceBuildPageView. export const TAB_PADDING_X = 16; -type TabsContextValue = { +type LinkTabsContextValue = { active: string; }; -const TabsContext = createContext(undefined); +const LinkTabsContext = createContext( + undefined, +); -type TabsProps = HTMLAttributes & TabsContextValue; +type LinkTabsProps = HTMLAttributes & LinkTabsContextValue; -export const Tabs: FC = ({ className, active, ...htmlProps }) => { +export const LinkTabs: FC = ({ + className, + active, + ...htmlProps +}) => { return ( - +
= ({ className, active, ...htmlProps }) => { )} {...htmlProps} /> - + ); }; -type TabsListProps = HTMLAttributes; +type LinkTabsListProps = HTMLAttributes; -export const TabsList: FC = ({ className, ...props }) => { - const tabsContext = useContext(TabsContext); +export const LinkTabsList: FC = ({ + className, + ...props +}) => { + const tabsContext = useContext(LinkTabsContext); const listRef = useRef(null); const indicatorRef = useRef(null); const hasInitialized = useRef(false); @@ -114,9 +201,9 @@ export const TabLink: FC = ({ className, ...linkProps }) => { - const tabsContext = useContext(TabsContext); + const tabsContext = useContext(LinkTabsContext); if (!tabsContext) { - throw new Error("Tab only can be used inside of Tabs"); + throw new Error("TabLink must be used inside LinkTabs"); } const isActive = tabsContext.active === value; diff --git a/site/src/modules/workspaces/WorkspaceBuildLogs/WorkspaceBuildLogs.tsx b/site/src/modules/workspaces/WorkspaceBuildLogs/WorkspaceBuildLogs.tsx index f5cec18745..9d667ea7bf 100644 --- a/site/src/modules/workspaces/WorkspaceBuildLogs/WorkspaceBuildLogs.tsx +++ b/site/src/modules/workspaces/WorkspaceBuildLogs/WorkspaceBuildLogs.tsx @@ -88,12 +88,10 @@ export const WorkspaceBuildLogs: FC = ({
= ({ >
{stage}
{shouldDisplayDuration && ( -
+
{duration} seconds
)}
- {!isEmpty && } + {!isEmpty && ( + + )} ); })} diff --git a/site/src/pages/DeploymentSettingsPage/NotificationsPage/NotificationsPage.tsx b/site/src/pages/DeploymentSettingsPage/NotificationsPage/NotificationsPage.tsx index b717ad59e1..38133891cd 100644 --- a/site/src/pages/DeploymentSettingsPage/NotificationsPage/NotificationsPage.tsx +++ b/site/src/pages/DeploymentSettingsPage/NotificationsPage/NotificationsPage.tsx @@ -1,4 +1,3 @@ -import type { Interpolation, Theme } from "@emotion/react"; import type { FC } from "react"; import { useQueries } from "react-query"; import { @@ -14,7 +13,12 @@ import { SettingsHeaderDocsLink, SettingsHeaderTitle, } from "#/components/SettingsHeader/SettingsHeader"; -import { TabLink, Tabs, TabsList } from "#/components/Tabs/Tabs"; +import { + Tabs, + TabsContent, + TabsList, + TabsTrigger, +} from "#/components/Tabs/Tabs"; import { useSearchParamsKey } from "#/hooks/useSearchParamsKey"; import { useDeploymentConfig } from "#/modules/management/DeploymentConfigProvider"; import { castNotificationMethod } from "#/modules/notifications/utils"; @@ -25,6 +29,14 @@ import OptionsTable from "../OptionsTable"; import { NotificationEvents } from "./NotificationEvents"; import { Troubleshooting } from "./Troubleshooting"; +const NOTIFICATION_TABS = ["events", "settings", "troubleshooting"] as const; + +function isNotificationTab( + value: string, +): value is (typeof NOTIFICATION_TABS)[number] { + return (NOTIFICATION_TABS as readonly string[]).includes(value); +} + const NotificationsPage: FC = () => { const { deploymentConfig } = useDeploymentConfig(); const [systemTemplatesByGroup, customTemplatesByGroup, dispatchMethods] = @@ -46,6 +58,10 @@ const NotificationsPage: FC = () => { defaultValue: "events", }); + const activeTab = isNotificationTab(tabState.value) + ? tabState.value + : NOTIFICATION_TABS[0]; + const ready = !!( systemTemplatesByGroup.data && customTemplatesByGroup.data && @@ -73,23 +89,16 @@ const NotificationsPage: FC = () => { - - - - Events - - - Settings - - - Troubleshooting - - - - -
- {ready ? ( - tabState.value === "events" ? ( + {!ready ? ( + + ) : ( + + + Events + Settings + Troubleshooting + + { castNotificationMethod, )} /> - ) : tabState.value === "troubleshooting" ? ( - - ) : ( + + deploymentGroupHasParent(o.group, "Notifications"), )} /> - ) - ) : ( - - )} -
+ + + + + + )} ); }; export default NotificationsPage; - -const styles = { - content: { paddingTop: 24 }, -} as Record>; diff --git a/site/src/pages/GroupsPage/GroupPage.tsx b/site/src/pages/GroupsPage/GroupPage.tsx index fb180787c8..03955f7748 100644 --- a/site/src/pages/GroupsPage/GroupPage.tsx +++ b/site/src/pages/GroupsPage/GroupPage.tsx @@ -29,7 +29,7 @@ import { SettingsHeaderDescription, SettingsHeaderTitle, } from "#/components/SettingsHeader/SettingsHeader"; -import { TabLink, Tabs, TabsList } from "#/components/Tabs/Tabs"; +import { LinkTabs, LinkTabsList, TabLink } from "#/components/Tabs/Tabs"; import { usePaginatedQuery } from "#/hooks/usePaginatedQuery"; import { pageTitle } from "#/utils/page"; @@ -135,16 +135,16 @@ const GroupPage: FC = () => {
{canUpdateGroup && ( - - + + Group members Group settings - - + + )} = ({ onSubmitGroupSyncSettings, onSubmitRoleSyncSettings, }) => { + const [_, setSearchParams] = useSearchParams(); const groupMappingCount = groupSyncSettings?.mapping ? Object.entries(groupSyncSettings.mapping).length : 0; @@ -60,39 +67,41 @@ const IdpSyncPageView: FC = ({ return (
{Boolean(error) && } - + { + setSearchParams({ tab: value }); + }} + > - - Group sync settings - - - Role sync settings - + Group sync settings + Role sync settings + + + + + + - {tab === "groups" ? ( - - ) : ( - - )}
); }; diff --git a/site/src/pages/TemplatePage/TemplateLayout.tsx b/site/src/pages/TemplatePage/TemplateLayout.tsx index a45c33cb28..2973ce8590 100644 --- a/site/src/pages/TemplatePage/TemplateLayout.tsx +++ b/site/src/pages/TemplatePage/TemplateLayout.tsx @@ -13,7 +13,7 @@ import type { AuthorizationRequest } from "#/api/typesGenerated"; import { ErrorAlert } from "#/components/Alert/ErrorAlert"; import { Loader } from "#/components/Loader/Loader"; import { Margins } from "#/components/Margins/Margins"; -import { TabLink, Tabs, TabsList } from "#/components/Tabs/Tabs"; +import { LinkTabs, LinkTabsList, TabLink } from "#/components/Tabs/Tabs"; import { useAuthenticated } from "#/hooks/useAuthenticated"; import { useFeatureVisibility } from "#/modules/dashboard/useFeatureVisibility"; import { @@ -135,9 +135,9 @@ export const TemplateLayout: FC = ({ }} /> - + - + Docs @@ -166,9 +166,9 @@ export const TemplateLayout: FC = ({ Prebuilds )} - + - + diff --git a/site/src/pages/WorkspaceBuildPage/Sidebar.tsx b/site/src/pages/WorkspaceBuildPage/Sidebar.tsx index 4e38d39cb2..9fa7b7963d 100644 --- a/site/src/pages/WorkspaceBuildPage/Sidebar.tsx +++ b/site/src/pages/WorkspaceBuildPage/Sidebar.tsx @@ -1,17 +1,16 @@ import type { FC, HTMLAttributes } from "react"; +import { cn } from "#/utils/cn"; + export const Sidebar: FC> = ({ children, ...attrs }) => { return (