From c4cbbd643ddf0888eb1e04ad3358c733c003f7fd Mon Sep 17 00:00:00 2001 From: "Hector A." Date: Thu, 8 Oct 2026 18:08:06 +0000 Subject: [PATCH] Add house IconButton and migrate the secondary bar off Primer React (#63742) --- .../tests/playwright-rendering.spec.ts | 17 ++ .../BreadcrumbsScroller.module.scss | 8 +- .../page-header/BreadcrumbsScroller.tsx | 2 +- .../page-header/DocsSecondaryBar.module.scss | 4 +- .../page-header/DocsSecondaryBar.tsx | 2 +- src/frame/components/page-header/Header.tsx | 4 +- .../ui/IconButton/IconButton.module.scss | 59 ++++++ .../components/ui/IconButton/IconButton.tsx | 55 +++++ src/frame/components/ui/IconButton/index.ts | 2 + .../ui/IconButton/tests/icon-button.ts | 68 +++++++ src/languages/components/LanguagePicker.tsx | 188 +++++++----------- 11 files changed, 283 insertions(+), 126 deletions(-) create mode 100644 src/frame/components/ui/IconButton/IconButton.module.scss create mode 100644 src/frame/components/ui/IconButton/IconButton.tsx create mode 100644 src/frame/components/ui/IconButton/index.ts create mode 100644 src/frame/components/ui/IconButton/tests/icon-button.ts diff --git a/src/fixtures/tests/playwright-rendering.spec.ts b/src/fixtures/tests/playwright-rendering.spec.ts index d59a4d672228..840d02190ad7 100644 --- a/src/fixtures/tests/playwright-rendering.spec.ts +++ b/src/fixtures/tests/playwright-rendering.spec.ts @@ -830,6 +830,23 @@ test.describe('test nav at different viewports', () => { await expect(page.getByTestId('sidebar')).toBeHidden() }) + test('secondary-bar icon buttons show a label tooltip that Escape dismisses', async ({ + page, + }) => { + await page.setViewportSize({ width: 1400, height: 700 }) + await page.goto('/get-started/foo/bar') + + const toggle = page.getByRole('button', { name: 'Collapse sidebar' }) + await expect(toggle).not.toHaveAttribute('aria-label') + const tooltip = page.locator(`[id="${await toggle.getAttribute('aria-labelledby')}"]`) + await expect(tooltip).toBeHidden() + await toggle.focus() + await expect(tooltip).toBeVisible() + await expect(tooltip).toHaveText('Collapse sidebar') + await page.keyboard.press('Escape') + await expect(tooltip).toBeHidden() + }) + for (const { name, width } of [ { name: 'medium viewports - 768-1011', width: 1000 }, { name: 'small viewports - 544-767', width: 555 }, diff --git a/src/frame/components/page-header/BreadcrumbsScroller.module.scss b/src/frame/components/page-header/BreadcrumbsScroller.module.scss index e2b62c3c8743..29ba9df9bca2 100644 --- a/src/frame/components/page-header/BreadcrumbsScroller.module.scss +++ b/src/frame/components/page-header/BreadcrumbsScroller.module.scss @@ -35,12 +35,12 @@ background-color: var(--brand-color-canvas-default); } -// Keep the solid canvas on hover because Primer's invisible IconButton uses a -// translucent tint that lets crumbs bleed through. A currentColor stroke thickens -// the fill-based octicon glyph for hover feedback. +// Keep the solid canvas on hover because the invisible variant's translucent tint +// lets crumbs bleed through. A currentColor stroke thickens the fill-based octicon +// glyph for hover feedback. .leftChevron:hover, .rightChevron:hover { - background-color: var(--brand-color-canvas-default) !important; + background-color: var(--brand-color-canvas-default); color: var(--brand-color-text-default, #000000); svg { diff --git a/src/frame/components/page-header/BreadcrumbsScroller.tsx b/src/frame/components/page-header/BreadcrumbsScroller.tsx index aa24dac7c9ef..521964c7b0aa 100644 --- a/src/frame/components/page-header/BreadcrumbsScroller.tsx +++ b/src/frame/components/page-header/BreadcrumbsScroller.tsx @@ -1,9 +1,9 @@ import { useCallback, useEffect, useRef, useState } from 'react' import type { FocusEvent } from 'react' import cx from 'clsx' -import { IconButton } from '@primer/react' import { ChevronLeftIcon, ChevronRightIcon } from '@primer/octicons-react' +import { IconButton } from '@/frame/components/ui/IconButton' import { useTranslation } from '@/languages/components/useTranslation' import { Breadcrumbs } from './Breadcrumbs' diff --git a/src/frame/components/page-header/DocsSecondaryBar.module.scss b/src/frame/components/page-header/DocsSecondaryBar.module.scss index e8e42fe5d98c..dbd3c214b843 100644 --- a/src/frame/components/page-header/DocsSecondaryBar.module.scss +++ b/src/frame/components/page-header/DocsSecondaryBar.module.scss @@ -65,10 +65,8 @@ } // Override primer/css color-fg-muted, whose grey clashes with brand-painted breadcrumbs. -// Keep !important because PRC's invisible IconButton variant uses .prc-Button-* rules -// that otherwise win. .toggleIcon { - color: var(--brand-color-text-muted) !important; + color: var(--brand-color-text-muted); } // The In this article row sits directly beneath the breadcrumb bar. DefaultLayout diff --git a/src/frame/components/page-header/DocsSecondaryBar.tsx b/src/frame/components/page-header/DocsSecondaryBar.tsx index ae45070cfe2a..71b7257083c2 100644 --- a/src/frame/components/page-header/DocsSecondaryBar.tsx +++ b/src/frame/components/page-header/DocsSecondaryBar.tsx @@ -1,8 +1,8 @@ import cx from 'clsx' import { useRouter } from 'next/router' -import { IconButton } from '@primer/react' import { SidebarCollapseIcon, SidebarExpandIcon } from '@primer/octicons-react' +import { IconButton } from '@/frame/components/ui/IconButton' import { useMainContext } from '@/frame/components/context/MainContext' import { useTranslation } from '@/languages/components/useTranslation' import { useSidebarCollapsed } from '@/frame/components/sidebar/SidebarCollapseContext' diff --git a/src/frame/components/page-header/Header.tsx b/src/frame/components/page-header/Header.tsx index 582d145f8daf..c97c3979c009 100644 --- a/src/frame/components/page-header/Header.tsx +++ b/src/frame/components/page-header/Header.tsx @@ -94,9 +94,7 @@ export const Header = ({ isNarrowMenuOpen, onNarrowMenuToggle }: Props) => { onClick={handleClick} leadingComponent={} trailingComponent={ - languagePickerVisible ? ( - - ) : undefined + languagePickerVisible ? : undefined } > , + 'aria-label' | 'children' | 'type' +> & { + icon: ElementType + 'aria-label': string + variant?: 'default' | 'invisible' + size?: 'small' | 'medium' + tooltip?: boolean + tooltipDirection?: 'n' | 'e' | 's' | 'w' +} + +// With a tooltip, the tooltip text labels the button through aria-labelledby, so +// the button carries no aria-label and screen readers hear one name. Hidden buttons +// skip the tooltip because nothing can hover or focus them. +export const IconButton = ({ + icon: Icon, + 'aria-label': ariaLabel, + variant = 'default', + size = 'medium', + tooltip = true, + tooltipDirection = 's', + className, + ...props +}: IconButtonProps) => { + const isHidden = props['aria-hidden'] === true || props['aria-hidden'] === 'true' + const withTooltip = tooltip && !isHidden + + const button = ( + + ) + + if (!withTooltip) return button + + return ( + + {button} + + ) +} diff --git a/src/frame/components/ui/IconButton/index.ts b/src/frame/components/ui/IconButton/index.ts new file mode 100644 index 000000000000..e44dae05560c --- /dev/null +++ b/src/frame/components/ui/IconButton/index.ts @@ -0,0 +1,2 @@ +export { IconButton } from '@/frame/components/ui/IconButton/IconButton' +export type { IconButtonProps } from '@/frame/components/ui/IconButton/IconButton' diff --git a/src/frame/components/ui/IconButton/tests/icon-button.ts b/src/frame/components/ui/IconButton/tests/icon-button.ts new file mode 100644 index 000000000000..32ff51d03c41 --- /dev/null +++ b/src/frame/components/ui/IconButton/tests/icon-button.ts @@ -0,0 +1,68 @@ +import { createElement } from 'react' +import { describe, expect, test } from 'vitest' +import { renderToStaticMarkup } from 'react-dom/server' +import { load } from 'cheerio' +import { SidebarExpandIcon } from '@primer/octicons-react' + +import { IconButton } from '@/frame/components/ui/IconButton' +import type { IconButtonProps } from '@/frame/components/ui/IconButton' + +const LABEL = 'Collapse sidebar' + +function render(props: Partial = {}) { + const html = renderToStaticMarkup( + createElement(IconButton, { icon: SidebarExpandIcon, 'aria-label': LABEL, ...props }), + ) + return load(html) +} + +describe('IconButton', () => { + test('labels the button through its tooltip, without a second aria-label', () => { + const $ = render() + const button = $('button') + expect(button.attr('aria-label')).toBeUndefined() + const labelId = button.attr('aria-labelledby') + expect(labelId).toBeTruthy() + const tooltip = $(`[id="${labelId}"]`) + expect(tooltip.text()).toBe(LABEL) + expect(tooltip.attr('aria-hidden')).toBe('true') + }) + + test('uses aria-label and renders no tooltip when the tooltip is off', () => { + const $ = render({ tooltip: false }) + expect($('button').attr('aria-label')).toBe(LABEL) + expect($('button').attr('aria-labelledby')).toBeUndefined() + expect($('div').length).toBe(0) + }) + + test('renders no tooltip for an aria-hidden button', () => { + const $ = render({ 'aria-hidden': true, tabIndex: -1 }) + expect($('button').attr('aria-label')).toBe(LABEL) + expect($('button').attr('aria-hidden')).toBe('true') + expect($('div').length).toBe(0) + }) + + test('is a type=button and passes through button attributes', () => { + const $ = render({ + 'data-testid': 'sidebar-collapse-toggle', + 'aria-expanded': true, + tabIndex: 0, + className: 'consumer', + } as Partial) + const button = $('button') + expect(button.attr('type')).toBe('button') + expect(button.attr('data-testid')).toBe('sidebar-collapse-toggle') + expect(button.attr('aria-expanded')).toBe('true') + expect(button.attr('tabindex')).toBe('0') + expect(button.hasClass('consumer')).toBe(true) + }) + + test('defaults to the default variant at medium size', () => { + const $ = render() + expect($('button').attr('data-variant')).toBe('default') + expect($('button').attr('data-size')).toBe('medium') + expect(render({ variant: 'invisible', size: 'small' })('button').attr('data-size')).toBe( + 'small', + ) + }) +}) diff --git a/src/languages/components/LanguagePicker.tsx b/src/languages/components/LanguagePicker.tsx index ee37a6c4f90e..00e914a3c4ab 100644 --- a/src/languages/components/LanguagePicker.tsx +++ b/src/languages/components/LanguagePicker.tsx @@ -1,21 +1,18 @@ import { DotFillIcon, GlobeIcon, TriangleDownIcon } from '@primer/octicons-react' import { useRouter } from 'next/router' -import { useState } from 'react' import type { KeyboardEvent } from 'react' import cx from 'clsx' import { useLanguages } from '@/languages/components/LanguagesContext' import { useUserLanguage } from '@/languages/components/useUserLanguage' -import { ActionList, ActionMenu, IconButton } from '@primer/react' import { ActionMenu as BrandActionMenu } from '@primer/react-brand' import { ActionMenuTrigger } from '@/frame/components/page-header/ActionMenuTrigger' -// The header variant shares its trigger, menu surface and rows with the plan/version +// The picker shares its trigger, menu surface and rows with the plan/version // picker so the two header dropdowns cannot drift apart. import styles from '@/frame/components/page-header/HeaderPicker.module.scss' type Props = { - variant?: 'default' | 'header' onNavigate?: () => void } @@ -24,12 +21,10 @@ type Props = { // CSS class names. const HEADER_TRIGGER_TESTID = 'language-picker-button' -export const LanguagePicker = ({ variant = 'default', onNavigate }: Props) => { +export const LanguagePicker = ({ onNavigate }: Props) => { const router = useRouter() const { languages } = useLanguages() const { setUserLanguageCookie } = useUserLanguage() - const [open, setOpen] = useState(false) - const isHeader = variant === 'header' const locale = router.locale || 'en' @@ -57,119 +52,84 @@ export const LanguagePicker = ({ variant = 'default', onNavigate }: Props) => { } } - if (isHeader) { - // Brand reports the chosen row by value, so navigation happens here. Rows stay - // list items: Brand's Overlay reads data-value on Enter and calls onSelect, - // while anchor rows close on Enter without following the link and never get - // aria-checked. - const handleSelect = (code: string) => { - if (!code) return - rememberLanguage(code) - // Brand owns open state; onNavigate closes the surrounding narrow menu. - onNavigate?.() - // locale: false stops Next adding a second locale prefix, matching Link. - router.push(languageHref(code), undefined, { locale: false }) - } + // Brand reports the chosen row by value, so navigation happens here. Rows stay + // list items: Brand's Overlay reads data-value on Enter and calls onSelect, + // while anchor rows close on Enter without following the link and never get + // aria-checked. + const handleSelect = (code: string) => { + if (!code) return + rememberLanguage(code) + // Brand owns open state; onNavigate closes the surrounding narrow menu. + onNavigate?.() + // locale: false stops Next adding a second locale prefix, matching Link. + router.push(languageHref(code), undefined, { locale: false }) + } - // Brand ActionMenu and SubdomainNavBar both listen for Escape on document and - // ignore defaultPrevented, so stop it in the capture phase to keep one Escape - // from closing both menus. Brand has no controlled open prop, so focus and - // click the trigger to close only the picker. - const handleEscapeCapture = (event: KeyboardEvent) => { - if (event.key !== 'Escape') return - - const trigger = event.currentTarget.querySelector( - `[data-testid="${HEADER_TRIGGER_TESTID}"]`, - ) - if (trigger?.getAttribute('aria-expanded') !== 'true') return - - event.preventDefault() - event.stopPropagation() - trigger.focus() - trigger.click() - } + // Brand ActionMenu and SubdomainNavBar both listen for Escape on document and + // ignore defaultPrevented, so stop it in the capture phase to keep one Escape + // from closing both menus. Brand has no controlled open prop, so focus and + // click the trigger to close only the picker. + const handleEscapeCapture = (event: KeyboardEvent) => { + if (event.key !== 'Escape') return - return ( -
- - } - trailingVisual={} - > - - {selectedLang.nativeName || selectedLang.name} - - - - {langs.map((lang) => ( - - - {lang.nativeName || lang.name} - - {/* Brand's check icon is hidden; the design uses a trailing dot. */} - {lang === selectedLang && ( - - )} - - ))} - - -
+ const trigger = event.currentTarget.querySelector( + `[data-testid="${HEADER_TRIGGER_TESTID}"]`, ) - } + if (trigger?.getAttribute('aria-expanded') !== 'true') return - // ActionList items support both breakpoint-specific menu behaviors. - const languageList = langs.map((lang) => ( - { - if (lang.code) { - rememberLanguage(lang.code) - } - setOpen(false) - onNavigate?.() - }} - > - {lang.nativeName || lang.name} - - )) + event.preventDefault() + event.stopPropagation() + trigger.focus() + trigger.click() + } return ( -
- - - - - - {languageList} - - +
+ + } + trailingVisual={} + > + + {selectedLang.nativeName || selectedLang.name} + + + + {langs.map((lang) => ( + + + {lang.nativeName || lang.name} + + {/* Brand's check icon is hidden; the design uses a trailing dot. */} + {lang === selectedLang && ( + + )} + + ))} + +
) }