From 59a22022de6962f145aa0ef8b6373e8586fc4c46 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Wed, 31 May 2023 11:56:09 -0500 Subject: [PATCH] Revert "Refactor FilteredActionList to address a11y violations and use new ActionList. (#3247)" This reverts commit e865e3e7abf4327bb5a1320d4d2ee42500d84277. --- .changeset/weak-jokes-chew.md | 5 -- docs/content/SelectPanel.mdx | 26 +++---- generated/components.json | 2 +- src/ActionList/List.tsx | 3 +- .../FilteredActionList.stories.tsx | 46 ++++++------ src/FilteredActionList/FilteredActionList.tsx | 70 +++++-------------- src/FilteredActionList/index.ts | 2 +- .../SelectPanel.features.stories.tsx | 50 +++++++------ src/SelectPanel/SelectPanel.stories.tsx | 49 +++++++------ src/SelectPanel/SelectPanel.test.tsx | 2 +- src/SelectPanel/SelectPanel.tsx | 23 +++--- src/drafts/MarkdownEditor/_SavedReplies.tsx | 14 +--- 12 files changed, 127 insertions(+), 165 deletions(-) delete mode 100644 .changeset/weak-jokes-chew.md diff --git a/.changeset/weak-jokes-chew.md b/.changeset/weak-jokes-chew.md deleted file mode 100644 index 1c2445cec20..00000000000 --- a/.changeset/weak-jokes-chew.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@primer/react": patch ---- - -FilteredActionList now uses new ActionList as a base, and SelectPanel reflects those changes. diff --git a/docs/content/SelectPanel.mdx b/docs/content/SelectPanel.mdx index b07b87c8731..0134547dccc 100644 --- a/docs/content/SelectPanel.mdx +++ b/docs/content/SelectPanel.mdx @@ -12,18 +12,20 @@ A `SelectPanel` provides an anchor that will open an overlay with a list of sele ```javascript live noinline function getColorCircle(color) { - return ( - - ) + return function () { + return ( + + ) + } } const items = [ diff --git a/generated/components.json b/generated/components.json index 4df9e9e8dcc..2bc6b888da9 100644 --- a/generated/components.json +++ b/generated/components.json @@ -3626,7 +3626,7 @@ "stories": [ { "id": "components-selectpanel--default", - "code": "() => {\n const [selected, setSelected] = React.useState([\n items[0],\n items[1],\n ])\n const [filter, setFilter] = React.useState('')\n const filteredItems = items.filter((item) =>\n item.text.toLowerCase().startsWith(filter.toLowerCase()),\n )\n const [open, setOpen] = useState(false)\n return (\n <>\n

Multi Select Panel

\n
Please select labels that describe your issue:
\n (\n \n {children ?? 'Select Labels'}\n \n )}\n placeholderText=\"Filter labels\"\n open={open}\n onOpenChange={setOpen}\n items={filteredItems}\n selected={selected}\n onSelectedChange={setSelected}\n onFilterChange={setFilter}\n overlayProps={{\n width: 'small',\n height: 'xsmall',\n }}\n />\n \n )\n}" + "code": "() => {\n const [selected, setSelected] = React.useState([\n items[0],\n items[1],\n ])\n const [filter, setFilter] = React.useState('')\n const filteredItems = items.filter((item) =>\n item.text.toLowerCase().startsWith(filter.toLowerCase()),\n )\n const [open, setOpen] = useState(false)\n return (\n <>\n

Multi Select Panel

\n
Please select labels that describe your issue:
\n (\n \n {children ?? 'Select Labels'}\n \n )}\n placeholderText=\"Filter labels\"\n open={open}\n onOpenChange={setOpen}\n items={filteredItems}\n selected={selected}\n onSelectedChange={setSelected}\n onFilterChange={setFilter}\n showItemDividers={true}\n overlayProps={{\n width: 'small',\n height: 'xsmall',\n }}\n />\n \n )\n}" } ], "props": [ diff --git a/src/ActionList/List.tsx b/src/ActionList/List.tsx index f9a779c6f56..9d0c1a8ba7c 100644 --- a/src/ActionList/List.tsx +++ b/src/ActionList/List.tsx @@ -69,8 +69,7 @@ export const List = React.forwardRef( value={{ variant, selectionVariant: selectionVariant || containerSelectionVariant, - // @ts-ignore showItemDividers may be passed by some components until next major. - showDividers: showDividers || !!props.showItemDividers, + showDividers, role: role || listRole, headingId, }} diff --git a/src/FilteredActionList/FilteredActionList.stories.tsx b/src/FilteredActionList/FilteredActionList.stories.tsx index ab1dbcc1b60..1b8d938c500 100644 --- a/src/FilteredActionList/FilteredActionList.stories.tsx +++ b/src/FilteredActionList/FilteredActionList.stories.tsx @@ -1,7 +1,7 @@ import {Meta} from '@storybook/react' import React from 'react' import {ThemeProvider} from '..' -import {FilteredActionList, ItemInput} from '../FilteredActionList' +import {FilteredActionList} from '../FilteredActionList' import BaseStyles from '../BaseStyles' import Box from '../Box' @@ -26,33 +26,35 @@ const meta: Meta = { export default meta function getColorCircle(color: string) { - return ( - - ) + return function () { + return ( + + ) + } } const items = [ - {leadingVisual: getColorCircle('#a2eeef'), text: 'enhancement', id: '1'}, - {leadingVisual: getColorCircle('#d73a4a'), text: 'bug', id: '2'}, - {leadingVisual: getColorCircle('#0cf478'), text: 'good first issue', id: '3'}, - {leadingVisual: getColorCircle('#ffd78e'), text: 'design', id: '4'}, - {leadingVisual: getColorCircle('#ff0000'), text: 'blocker', id: '5'}, - {leadingVisual: getColorCircle('#a4f287'), text: 'backend', id: '6'}, - {leadingVisual: getColorCircle('#8dc6fc'), text: 'frontend', id: '7'}, -] as ItemInput[] + {leadingVisual: getColorCircle('#a2eeef'), text: 'enhancement', id: 1}, + {leadingVisual: getColorCircle('#d73a4a'), text: 'bug', id: 2}, + {leadingVisual: getColorCircle('#0cf478'), text: 'good first issue', id: 3}, + {leadingVisual: getColorCircle('#ffd78e'), text: 'design', id: 4}, + {leadingVisual: getColorCircle('#ff0000'), text: 'blocker', id: 5}, + {leadingVisual: getColorCircle('#a4f287'), text: 'backend', id: 6}, + {leadingVisual: getColorCircle('#8dc6fc'), text: 'frontend', id: 7}, +] export function Default(): JSX.Element { const [filter, setFilter] = React.useState('') - const filteredItems = items.filter(item => item.text?.toLowerCase().startsWith(filter.toLowerCase())) + const filteredItems = items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) return ( <> diff --git a/src/FilteredActionList/FilteredActionList.tsx b/src/FilteredActionList/FilteredActionList.tsx index 670d29a8d63..00fc8ae8bb6 100644 --- a/src/FilteredActionList/FilteredActionList.tsx +++ b/src/FilteredActionList/FilteredActionList.tsx @@ -1,42 +1,33 @@ +import type {ScrollIntoViewOptions} from '@primer/behaviors' +import {scrollIntoView} from '@primer/behaviors' import React, {KeyboardEventHandler, useCallback, useEffect, useRef} from 'react' -import TextInput, {TextInputProps} from '../TextInput' +import styled from 'styled-components' import Box from '../Box' -import {ActionList, ActionListProps, ActionListItemProps} from '../ActionList' import Spinner from '../Spinner' -import {useFocusZone} from '../hooks/useFocusZone' -import {useProvidedStateOrCreate} from '../hooks/useProvidedStateOrCreate' -import styled from 'styled-components' +import TextInput, {TextInputProps} from '../TextInput' import {get} from '../constants' +import {ActionList} from '../deprecated/ActionList' +import {GroupedListProps, ListPropsBase} from '../deprecated/ActionList/List' +import {useFocusZone} from '../hooks/useFocusZone' +import {useId} from '../hooks/useId' import {useProvidedRefOrCreate} from '../hooks/useProvidedRefOrCreate' +import {useProvidedStateOrCreate} from '../hooks/useProvidedStateOrCreate' import useScrollFlash from '../hooks/useScrollFlash' -import {scrollIntoView} from '@primer/behaviors' -import type {ScrollIntoViewOptions} from '@primer/behaviors' -import {useId} from '../hooks/useId' import {VisuallyHidden} from '../internal/components/VisuallyHidden' import {SxProp} from '../sx' const menuScrollMargins: ScrollIntoViewOptions = {startMargin: 0, endMargin: 8} -export type ItemInput = Partial< - ActionListItemProps & { - description?: string | React.ReactElement - descriptionVariant?: 'inline' | 'block' - leadingVisual?: JSX.Element - onAction?: (itemFromAction: ItemInput, event: React.MouseEvent) => void - selected?: boolean - text?: string - trailingVisual?: string - } -> - -export interface FilteredActionListProps extends ActionListProps, SxProp { +export interface FilteredActionListProps + extends Partial>, + ListPropsBase, + SxProp { loading?: boolean placeholderText?: string filterValue?: string onFilterChange: (value: string, e: React.ChangeEvent) => void textInputProps?: Partial> inputRef?: React.RefObject - items: ItemInput[] } const StyledHeader = styled.div` @@ -44,27 +35,6 @@ const StyledHeader = styled.div` z-index: 1; ` -const renderFn = ({ - description, - descriptionVariant, - id, - sx, - text, - trailingVisual, - leadingVisual, - onSelect, - selected, -}: ItemInput): React.ReactElement => { - return ( - - {!!leadingVisual && {leadingVisual}} - {text ? text : null} - {description ? {description} : null} - {!!trailingVisual && {trailingVisual}} - - ) -} - export function FilteredActionList({ loading = false, placeholderText, @@ -87,7 +57,7 @@ export function FilteredActionList({ ) const scrollContainerRef = useRef(null) - const listContainerRef = useRef(null) + const listContainerRef = useRef(null) const inputRef = useProvidedRefOrCreate(providedInputRef) const activeDescendantRef = useRef() const listId = useId() @@ -114,7 +84,7 @@ export function FilteredActionList({ return !(element instanceof HTMLInputElement) }, activeDescendantFocus: inputRef, - onActiveDescendantChanged: (current, _previous, directlyActivated) => { + onActiveDescendantChanged: (current, previous, directlyActivated) => { activeDescendantRef.current = current if (current && scrollContainerRef.current && directlyActivated) { @@ -162,15 +132,7 @@ export function FilteredActionList({ ) : ( - - {items.map(i => renderFn(i))} - + )} diff --git a/src/FilteredActionList/index.ts b/src/FilteredActionList/index.ts index 8a96b74fd6f..3f8176fe71c 100644 --- a/src/FilteredActionList/index.ts +++ b/src/FilteredActionList/index.ts @@ -1,2 +1,2 @@ export {FilteredActionList} from './FilteredActionList' -export type {FilteredActionListProps, ItemInput} from './FilteredActionList' +export type {FilteredActionListProps} from './FilteredActionList' diff --git a/src/SelectPanel/SelectPanel.features.stories.tsx b/src/SelectPanel/SelectPanel.features.stories.tsx index b833a8bb614..42b4f6ccf24 100644 --- a/src/SelectPanel/SelectPanel.features.stories.tsx +++ b/src/SelectPanel/SelectPanel.features.stories.tsx @@ -3,7 +3,7 @@ import {ComponentMeta} from '@storybook/react' import Box from '../Box' import {Button} from '../Button' -import {ItemInput} from '../FilteredActionList' +import {ItemInput} from '../deprecated/ActionList/List' import {SelectPanel} from './SelectPanel' import {TriangleDownIcon} from '@primer/octicons-react' import type {OverlayProps} from '../Overlay' @@ -14,28 +14,30 @@ export default { } as ComponentMeta function getColorCircle(color: string) { - return ( - - ) + return function () { + return ( + + ) + } } const items = [ - {leadingVisual: getColorCircle('#a2eeef'), text: 'enhancement', id: '1'}, - {leadingVisual: getColorCircle('#d73a4a'), text: 'bug', id: '2'}, - {leadingVisual: getColorCircle('#0cf478'), text: 'good first issue', id: '3'}, - {leadingVisual: getColorCircle('#ffd78e'), text: 'design', id: '4'}, - {leadingVisual: getColorCircle('#ff0000'), text: 'blocker', id: '5'}, - {leadingVisual: getColorCircle('#a4f287'), text: 'backend', id: '6'}, - {leadingVisual: getColorCircle('#8dc6fc'), text: 'frontend', id: '7'}, + {leadingVisual: getColorCircle('#a2eeef'), text: 'enhancement', id: 1}, + {leadingVisual: getColorCircle('#d73a4a'), text: 'bug', id: 2}, + {leadingVisual: getColorCircle('#0cf478'), text: 'good first issue', id: 3}, + {leadingVisual: getColorCircle('#ffd78e'), text: 'design', id: 4}, + {leadingVisual: getColorCircle('#ff0000'), text: 'blocker', id: 5}, + {leadingVisual: getColorCircle('#a4f287'), text: 'backend', id: 6}, + {leadingVisual: getColorCircle('#8dc6fc'), text: 'frontend', id: 7}, ] export const SingleSelectStory = () => { @@ -61,7 +63,6 @@ export const SingleSelectStory = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} - showDividers={true} showItemDividers={true} overlayProps={{width: 'small', height: 'xsmall'}} /> @@ -93,6 +94,7 @@ export const ExternalAnchorStory = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} + showItemDividers={true} overlayProps={{width: 'small', height: 'xsmall'}} /> @@ -123,6 +125,7 @@ export const SelectPanelHeightInitialWithOverflowingItemsStory = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} + showItemDividers={true} overlayProps={{width: 'small', height: 'initial', maxHeight: 'xsmall'}} /> @@ -154,6 +157,7 @@ export const SelectPanelHeightInitialWithUnderflowingItemsStory = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} + showItemDividers={true} overlayProps={{width: 'small', height: 'initial', maxHeight: 'xsmall'}} /> @@ -198,6 +202,7 @@ export const SelectPanelHeightInitialWithUnderflowingItemsAfterFetch = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} + showItemDividers={true} overlayProps={{width: 'small', height, maxHeight: 'xsmall'}} /> @@ -229,6 +234,7 @@ export const SelectPanelAboveTallBody = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} + showItemDividers={true} overlayProps={{width: 'small', height: 'xsmall'}} />
{ selected={selectedA} onSelectedChange={setSelectedA} onFilterChange={setFilter} + showItemDividers={true} overlayProps={{height: 'medium'}} />

With height:auto, maxheight:medium

@@ -286,6 +293,7 @@ export const SelectPanelHeightAndScroll = () => { selected={selectedB} onSelectedChange={setSelectedB} onFilterChange={setFilter} + showItemDividers={true} overlayProps={{ height: 'auto', maxHeight: 'medium', diff --git a/src/SelectPanel/SelectPanel.stories.tsx b/src/SelectPanel/SelectPanel.stories.tsx index a755207e390..1ab7d29b47b 100644 --- a/src/SelectPanel/SelectPanel.stories.tsx +++ b/src/SelectPanel/SelectPanel.stories.tsx @@ -2,10 +2,10 @@ import {TriangleDownIcon} from '@primer/octicons-react' import {ComponentMeta} from '@storybook/react' import React, {useState} from 'react' +import Box from '../Box' import {Button} from '../Button' import {SelectPanel} from '../SelectPanel' -import Box from '../Box' -import {ItemInput} from '../FilteredActionList' +import {ItemInput} from '../deprecated/ActionList/List' export default { title: 'Components/SelectPanel', @@ -13,30 +13,32 @@ export default { } as ComponentMeta function getColorCircle(color: string) { - return ( - - ) + return function () { + return ( + + ) + } } const items = [ - {leadingVisual: getColorCircle('#a2eeef'), text: 'enhancement', id: '1'}, - {leadingVisual: getColorCircle('#d73a4a'), text: 'bug', id: '2'}, - {leadingVisual: getColorCircle('#0cf478'), text: 'good first issue', id: '3'}, - {leadingVisual: getColorCircle('#ffd78e'), text: 'design', id: '4'}, - {leadingVisual: getColorCircle('#ff0000'), text: 'blocker', id: '5'}, - {leadingVisual: getColorCircle('#a4f287'), text: 'backend', id: '6'}, - {leadingVisual: getColorCircle('#8dc6fc'), text: 'frontend', id: '7'}, + {leadingVisual: getColorCircle('#a2eeef'), text: 'enhancement', id: 1}, + {leadingVisual: getColorCircle('#d73a4a'), text: 'bug', id: 2}, + {leadingVisual: getColorCircle('#0cf478'), text: 'good first issue', id: 3}, + {leadingVisual: getColorCircle('#ffd78e'), text: 'design', id: 4}, + {leadingVisual: getColorCircle('#ff0000'), text: 'blocker', id: 5}, + {leadingVisual: getColorCircle('#a4f287'), text: 'backend', id: 6}, + {leadingVisual: getColorCircle('#8dc6fc'), text: 'frontend', id: 7}, ] export const Default = () => { @@ -63,6 +65,7 @@ export const Default = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} + showItemDividers={true} overlayProps={{width: 'small', height: 'xsmall'}} /> diff --git a/src/SelectPanel/SelectPanel.test.tsx b/src/SelectPanel/SelectPanel.test.tsx index 551d742aaa3..05332623326 100644 --- a/src/SelectPanel/SelectPanel.test.tsx +++ b/src/SelectPanel/SelectPanel.test.tsx @@ -5,7 +5,7 @@ import theme from '../theme' import {SelectPanel} from '../SelectPanel' import {behavesAsComponent, checkExports} from '../utils/testing' import {BaseStyles, SSRProvider, ThemeProvider} from '..' -import {ItemInput} from '../FilteredActionList' +import {ItemInput} from '../deprecated/ActionList/List' expect.extend(toHaveNoViolations) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index 02fd71726d3..dfcac4cdd97 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -1,16 +1,17 @@ import {SearchIcon} from '@primer/octicons-react' import React, {useCallback, useMemo} from 'react' -import {FilteredActionList, FilteredActionListProps, ItemInput} from '../FilteredActionList' -import {OverlayProps} from '../Overlay' -import {FocusZoneHookSettings} from '../hooks/useFocusZone' -import {DropdownButton} from '../deprecated/DropdownMenu' -import {ActionListItemProps} from '../ActionList' import {AnchoredOverlay, AnchoredOverlayProps} from '../AnchoredOverlay' import {AnchoredOverlayWrapperAnchorProps} from '../AnchoredOverlay/AnchoredOverlay' import Box from '../Box' +import {FilteredActionList, FilteredActionListProps} from '../FilteredActionList' import Heading from '../Heading' +import {OverlayProps} from '../Overlay' import {TextInputProps} from '../TextInput' +import {ItemProps} from '../deprecated/ActionList' +import {ItemInput} from '../deprecated/ActionList/List' +import {DropdownButton} from '../deprecated/DropdownMenu' import {useProvidedRefOrCreate} from '../hooks' +import {FocusZoneHookSettings} from '../hooks/useFocusZone' import {useId} from '../hooks/useId' import {useProvidedStateOrCreate} from '../hooks/useProvidedStateOrCreate' import {LiveRegion, LiveRegionOutlet, Message} from '../internal/components/LiveRegion' @@ -42,8 +43,7 @@ export type SelectPanelProps = SelectPanelBaseProps & Omit & Pick & AnchoredOverlayWrapperAnchorProps & - // TODO: 23-05-23 - Remove showItemDividers after next-major release - (SelectPanelSingleSelection | SelectPanelMultiSelection) & {showItemDividers?: boolean} + (SelectPanelSingleSelection | SelectPanelMultiSelection) function isMultiSelectVariant( selected: SelectPanelSingleSelection['selected'] | SelectPanelMultiSelection['selected'], @@ -120,7 +120,9 @@ export function SelectPanel({ ...item, role: 'option', selected: 'selected' in item && item.selected === undefined ? undefined : isItemSelected, - onSelect: (event: React.MouseEvent | React.KeyboardEvent) => { + onAction: (itemFromAction, event) => { + item.onAction?.(itemFromAction, event) + if (event.defaultPrevented) { return } @@ -139,7 +141,7 @@ export function SelectPanel({ singleSelectOnChange(item === selected ? undefined : item) onClose('selection') }, - } as ActionListItemProps + } as ItemProps }) }, [onClose, onSelectedChange, items, selected]) @@ -189,10 +191,11 @@ export function SelectPanel({ { .map( (reply, i): Item => ({ text: reply.name, - description: ( - - {reply.content} - - ), + description: reply.content, descriptionVariant: 'block', trailingVisual: i < 9 ? `Ctrl + ${i + 1}` : undefined, sx: { @@ -71,7 +66,6 @@ export const SavedRepliesButton = () => { maxWidth: '100%', }, }, - id: i.toString(), }), ) @@ -111,12 +105,6 @@ export const SavedRepliesButton = () => { onSelectItem(Array.isArray(selection) ? selection[0] : selection) }} overlayProps={{width: 'small', maxHeight: 'small', anchorSide: 'outside-right', onKeyDown}} - // @ts-ignore this is bad because SelectPanel does not accept selectionVariant in the public API - // but it does pass it down to FilteredActionList underneath. - // SavedReplies should not use SelectPanel and override it's semantics, it should instead - // use the building blocks of SelectPanel to build a new component - selectionVariant={undefined} - aria-multiselectable={undefined} /> ) : ( <>