From ff93686ea72ac214bb78755900ab1b3b75c76a6b Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Fri, 24 Jan 2025 12:10:59 -0800 Subject: [PATCH 1/4] Fix experimental SelectPanel anchoring behavior --- .../SelectPanel2/SelectPanel.module.css | 8 ++++++ .../experimental/SelectPanel2/SelectPanel.tsx | 26 +++++++++++++++++++ .../react/src/hooks/useAnchoredPosition.ts | 6 ++++- 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/packages/react/src/experimental/SelectPanel2/SelectPanel.module.css b/packages/react/src/experimental/SelectPanel2/SelectPanel.module.css index 4a7feda9c3d..5dd88effaf3 100644 --- a/packages/react/src/experimental/SelectPanel2/SelectPanel.module.css +++ b/packages/react/src/experimental/SelectPanel2/SelectPanel.module.css @@ -8,6 +8,14 @@ --position-top: 0; --position-left: 0; + &[data-visibility='visible'] { + visibility: visible; + } + + &[data-visibility='hidden'] { + visibility: hidden; + } + &:where([open]) { display: flex; /* to fit children */ } diff --git a/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx b/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx index d4c71e629be..422d787a4dc 100644 --- a/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx +++ b/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx @@ -29,6 +29,7 @@ import {clsx} from 'clsx' import {useFeatureFlag} from '../../FeatureFlags' import classes from './SelectPanel.module.css' +import type {PositionSettings} from '@primer/behaviors' const CSS_MODULES_FEATURE_FLAG = 'primer_react_css_modules_ga' @@ -66,6 +67,7 @@ export type SelectPanelProps = { defaultOpen?: boolean open?: boolean anchorRef?: React.RefObject + anchoredPositionSettings?: Partial onCancel?: () => void onClearSelection?: undefined | (() => void) @@ -89,6 +91,7 @@ const Panel: React.FC = ({ defaultOpen = false, open: propsOpen, anchorRef: providedAnchorRef, + anchoredPositionSettings, onCancel: propsOnCancel, onClearSelection: propsOnClearSelection, @@ -228,6 +231,7 @@ const Panel: React.FC = ({ floatingElementRef: dialogRef, side: 'outside-bottom', align: 'start', + ...anchoredPositionSettings, }, [internalOpen, anchorRef.current, dialogRef.current], ) @@ -245,6 +249,20 @@ const Panel: React.FC = ({ maxHeightValue = '100vh' } + const [isVisible, setIsVisible] = useState(internalOpen) + + useEffect(() => { + if (internalOpen) { + // give the browser time to render the panel and for useAnchoredPosition + // to calculate its actual size + window.requestAnimationFrame(() => { + setIsVisible(true) + }) + } else { + setIsVisible(false) + } + }, [internalOpen, setIsVisible]) + return ( <> {Anchor} @@ -258,10 +276,18 @@ const Panel: React.FC = ({ height="fit-content" maxHeight={maxHeight} data-variant={currentVariant} + data-visibility={isVisible ? 'visible' : 'hidden'} sx={ enabled ? undefined : { + '&[data-visibility="visible"]': { + visibility: 'visible', + }, + '&[data-visibility="hidden"]': { + visibility: 'hidden', + }, + '--max-height': heightMap[maxHeight], // reset dialog default styles border: 'none', diff --git a/packages/react/src/hooks/useAnchoredPosition.ts b/packages/react/src/hooks/useAnchoredPosition.ts index 6365a23cd27..b8f0c18e093 100644 --- a/packages/react/src/hooks/useAnchoredPosition.ts +++ b/packages/react/src/hooks/useAnchoredPosition.ts @@ -1,4 +1,4 @@ -import React from 'react' +import React, {type RefObject} from 'react' import {getAnchoredPosition} from '@primer/behaviors' import type {AnchorPosition, PositionSettings} from '@primer/behaviors' import {useProvidedRefOrCreate} from './useProvidedRefOrCreate' @@ -45,8 +45,12 @@ export function useAnchoredPosition( useLayoutEffect(updatePosition, [updatePosition]) + // recalculate position if viewport changes size useResizeObserver(updatePosition) + // recalculate position if the floating element changes size + useResizeObserver(updatePosition, floatingElementRef as RefObject) + return { floatingElementRef, anchorElementRef, From 916b9881363ca911253c82fca83790ab55f0a6bd Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Fri, 24 Jan 2025 14:16:07 -0800 Subject: [PATCH 2/4] Simplify --- .../SelectPanel2/SelectPanel.module.css | 8 ----- .../experimental/SelectPanel2/SelectPanel.tsx | 30 ++++--------------- .../react/src/hooks/useAnchoredPosition.ts | 6 +--- 3 files changed, 7 insertions(+), 37 deletions(-) diff --git a/packages/react/src/experimental/SelectPanel2/SelectPanel.module.css b/packages/react/src/experimental/SelectPanel2/SelectPanel.module.css index 5dd88effaf3..4a7feda9c3d 100644 --- a/packages/react/src/experimental/SelectPanel2/SelectPanel.module.css +++ b/packages/react/src/experimental/SelectPanel2/SelectPanel.module.css @@ -8,14 +8,6 @@ --position-top: 0; --position-left: 0; - &[data-visibility='visible'] { - visibility: visible; - } - - &[data-visibility='hidden'] { - visibility: hidden; - } - &:where([open]) { display: flex; /* to fit children */ } diff --git a/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx b/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx index 422d787a4dc..47d44d4235f 100644 --- a/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx +++ b/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx @@ -249,20 +249,6 @@ const Panel: React.FC = ({ maxHeightValue = '100vh' } - const [isVisible, setIsVisible] = useState(internalOpen) - - useEffect(() => { - if (internalOpen) { - // give the browser time to render the panel and for useAnchoredPosition - // to calculate its actual size - window.requestAnimationFrame(() => { - setIsVisible(true) - }) - } else { - setIsVisible(false) - } - }, [internalOpen, setIsVisible]) - return ( <> {Anchor} @@ -276,24 +262,15 @@ const Panel: React.FC = ({ height="fit-content" maxHeight={maxHeight} data-variant={currentVariant} - data-visibility={isVisible ? 'visible' : 'hidden'} sx={ enabled ? undefined : { - '&[data-visibility="visible"]': { - visibility: 'visible', - }, - '&[data-visibility="hidden"]': { - visibility: 'hidden', - }, - '--max-height': heightMap[maxHeight], // reset dialog default styles border: 'none', padding: 0, color: 'fg.default', - '&[open]': {display: 'flex'}, // to fit children '&[data-variant="anchored"], &[data-variant="full-screen"]': { margin: 0, @@ -335,8 +312,13 @@ const Panel: React.FC = ({ '--max-height': maxHeightValue, '--position-top': `${position?.top ?? 0}px`, '--position-left': `${position?.left ?? 0}px`, + visibility: internalOpen ? 'visible' : 'hidden', + display: 'flex', } as React.CSSProperties) - : undefined + : { + visibility: internalOpen ? 'visible' : 'hidden', + display: 'flex', + } } className={enabled ? classes.Overlay : undefined} {...props} diff --git a/packages/react/src/hooks/useAnchoredPosition.ts b/packages/react/src/hooks/useAnchoredPosition.ts index b8f0c18e093..6365a23cd27 100644 --- a/packages/react/src/hooks/useAnchoredPosition.ts +++ b/packages/react/src/hooks/useAnchoredPosition.ts @@ -1,4 +1,4 @@ -import React, {type RefObject} from 'react' +import React from 'react' import {getAnchoredPosition} from '@primer/behaviors' import type {AnchorPosition, PositionSettings} from '@primer/behaviors' import {useProvidedRefOrCreate} from './useProvidedRefOrCreate' @@ -45,12 +45,8 @@ export function useAnchoredPosition( useLayoutEffect(updatePosition, [updatePosition]) - // recalculate position if viewport changes size useResizeObserver(updatePosition) - // recalculate position if the floating element changes size - useResizeObserver(updatePosition, floatingElementRef as RefObject) - return { floatingElementRef, anchorElementRef, From f9ca1f265d8abe2cacd0ffe212ae1a2fc34aeabe Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Mon, 27 Jan 2025 09:41:55 -0800 Subject: [PATCH 3/4] Add changeset --- .changeset/gentle-planets-grab.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/gentle-planets-grab.md diff --git a/.changeset/gentle-planets-grab.md b/.changeset/gentle-planets-grab.md new file mode 100644 index 00000000000..d0ffd564f06 --- /dev/null +++ b/.changeset/gentle-planets-grab.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Fix experimental SelectPanel anchoring behavior From 244b4911c2060465823da3262d603e8c2e67984a Mon Sep 17 00:00:00 2001 From: Marie Lucca <40550942+francinelucca@users.noreply.github.com> Date: Mon, 27 Jan 2025 23:49:59 -0500 Subject: [PATCH 4/4] =?UTF-8?q?Revert=20"[Accessibility][Storybook]=20Add?= =?UTF-8?q?=20aria-labels=20to=20the=20multiple=20item=20progr=E2=80=A6"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 65f89fedafed9b736e2f5da549cf552da2dfb90f. --- .changeset/orange-roses-give.md | 5 -- .../ProgressBar.features.stories.tsx | 8 ++-- .../src/ProgressBar/ProgressBar.figma.tsx | 2 +- .../src/ProgressBar/ProgressBar.stories.tsx | 7 ++- .../react/src/ProgressBar/ProgressBar.tsx | 24 +--------- .../react/src/__tests__/ProgressBar.test.tsx | 47 +++++-------------- 6 files changed, 23 insertions(+), 70 deletions(-) delete mode 100644 .changeset/orange-roses-give.md diff --git a/.changeset/orange-roses-give.md b/.changeset/orange-roses-give.md deleted file mode 100644 index 38cf304ceed..00000000000 --- a/.changeset/orange-roses-give.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@primer/react": minor ---- - -In dev mode, warn users to add an aria-label to ProgressBar.Item if the ProgressBar.Item is not aria-hidden. diff --git a/packages/react/src/ProgressBar/ProgressBar.features.stories.tsx b/packages/react/src/ProgressBar/ProgressBar.features.stories.tsx index 0e79793d71a..145dae260e7 100644 --- a/packages/react/src/ProgressBar/ProgressBar.features.stories.tsx +++ b/packages/react/src/ProgressBar/ProgressBar.features.stories.tsx @@ -19,10 +19,10 @@ export const Inline = () => export const MultipleItems = () => ( - - - - + + + + ) diff --git a/packages/react/src/ProgressBar/ProgressBar.figma.tsx b/packages/react/src/ProgressBar/ProgressBar.figma.tsx index cbff1ef8409..f30cbc928b1 100644 --- a/packages/react/src/ProgressBar/ProgressBar.figma.tsx +++ b/packages/react/src/ProgressBar/ProgressBar.figma.tsx @@ -38,6 +38,6 @@ figma.connect( gray: 'neutral.epmhasis', }), }, - example: ({color}) => , + example: ({color}) => , }, ) diff --git a/packages/react/src/ProgressBar/ProgressBar.stories.tsx b/packages/react/src/ProgressBar/ProgressBar.stories.tsx index 725fa9f9ed2..356f22cc116 100644 --- a/packages/react/src/ProgressBar/ProgressBar.stories.tsx +++ b/packages/react/src/ProgressBar/ProgressBar.stories.tsx @@ -1,7 +1,6 @@ import React, {useEffect} from 'react' import type {Meta} from '@storybook/react' -import {ProgressBar} from '..' -import type {ProgressBarProps} from './ProgressBar' +import {ProgressBar, type ProgressBarProps} from '..' const sectionColorsDefault = [ 'success.emphasis', @@ -32,9 +31,9 @@ export const Playground = ({sections, ...args}: ProgressBarProps & {sections: nu return } else { return ( - + {[...Array(sections).keys()].map(i => ( - + ))} ) diff --git a/packages/react/src/ProgressBar/ProgressBar.tsx b/packages/react/src/ProgressBar/ProgressBar.tsx index 46065221327..d601669dbb6 100644 --- a/packages/react/src/ProgressBar/ProgressBar.tsx +++ b/packages/react/src/ProgressBar/ProgressBar.tsx @@ -73,6 +73,7 @@ const ProgressContainer = toggleStyledComponent( ) export type ProgressBarItems = React.HTMLAttributes & { + 'aria-label'?: string className?: string } & ProgressProp & SxProp @@ -82,7 +83,6 @@ export const Item = forwardRef( { progress, 'aria-label': ariaLabel, - 'aria-hidden': ariaHidden, 'aria-valuenow': ariaValueNow, 'aria-valuetext': ariaValueText, className, @@ -110,24 +110,6 @@ export const Item = forwardRef( styles[progressBarWidth] = progress ? `${progress}%` : '0%' styles[progressBarBg] = (bgType && `var(--bgColor-${bgType[0]}-${bgType[1]})`) || 'var(--bgColor-success-emphasis)' - if (__DEV__) { - /** - * The Linter yells because it thinks this conditionally calls an effect, - * but since this is a compile-time flag and not a runtime conditional - * this is safe, and ensures the entire effect is kept out of prod builds - * shaving precious bytes from the output, and avoiding mounting a noop effect - */ - // eslint-disable-next-line react-hooks/rules-of-hooks - React.useEffect(() => { - if (!ariaHidden && !ariaLabel) { - // eslint-disable-next-line no-console - console.warn( - 'This component should include an aria-label or should be aria-hidden if surrounding text can be used to perceive the progress.', - ) - } - }, [ariaHidden, ariaLabel]) - } - return ( ( 'aria-label': ariaLabel, 'aria-valuenow': ariaValueNow, 'aria-valuetext': ariaValueText, - 'aria-hidden': ariaHidden, className, ...rest }: ProgressBarProps, @@ -194,10 +175,9 @@ export const ProgressBar = forwardRef( )} diff --git a/packages/react/src/__tests__/ProgressBar.test.tsx b/packages/react/src/__tests__/ProgressBar.test.tsx index f11953f91e2..ead938b3e18 100644 --- a/packages/react/src/__tests__/ProgressBar.test.tsx +++ b/packages/react/src/__tests__/ProgressBar.test.tsx @@ -6,10 +6,7 @@ import axe from 'axe-core' import {FeatureFlags} from '../FeatureFlags' describe('ProgressBar', () => { - behavesAsComponent({ - Component: ProgressBar, - toRender: () => , - }) + behavesAsComponent({Component: ProgressBar, toRender: () => }) checkExports('ProgressBar', { default: undefined, @@ -83,15 +80,24 @@ describe('ProgressBar', () => { }) it('passed the `aria-valuenow` down to the progress bar', () => { - const {getByRole} = HTMLRender() + const {getByRole} = HTMLRender() expect(getByRole('progressbar')).toHaveAttribute('aria-valuenow', '80') }) it('passed the `aria-valuetext` down to the progress bar', () => { - const {getByRole} = HTMLRender() + const {getByRole} = HTMLRender() expect(getByRole('progressbar')).toHaveAttribute('aria-valuetext', '80 percent') }) + it('does not pass the `aria-label` down to the progress bar if there are multiple items', () => { + const {getByRole} = HTMLRender( + + + , + ) + expect(getByRole('progressbar')).not.toHaveAttribute('aria-label') + }) + it('passes aria attributes to the progress bar item', () => { const {getByRole} = HTMLRender( @@ -103,7 +109,7 @@ describe('ProgressBar', () => { }) it('provides `aria-valuenow` to the progress bar item if it is not already provided', () => { - const {getByRole} = HTMLRender() + const {getByRole} = HTMLRender() expect(getByRole('progressbar')).toHaveAttribute('aria-valuenow', '50') }) @@ -112,31 +118,4 @@ describe('ProgressBar', () => { expect(getByRole('progressbar')).toHaveAttribute('aria-valuenow', '0') }) - - describe('console.warn', () => { - const mockWarningFn = jest.fn() - - beforeEach(() => { - jest.spyOn(global.console, 'warn').mockImplementation(mockWarningFn) - }) - - afterEach(() => { - jest.clearAllMocks() - }) - - it('should warn users if aria-label is not provided', () => { - HTMLRender() - expect(mockWarningFn).toHaveBeenCalled() - }) - - it('should not warn users if aria-label is not provided but aria-hidden is', () => { - HTMLRender() - expect(mockWarningFn).not.toHaveBeenCalled() - }) - - it('should not warn users if aria-label is provided', () => { - HTMLRender() - expect(mockWarningFn).not.toHaveBeenCalled() - }) - }) })