From 78b01ad2791c3c5f322504c7c789321692f4cb6c Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Tue, 13 Jun 2023 08:09:57 +1000 Subject: [PATCH] Revert "Revert "Refactor FilteredActionList to address a11y violations and use new ActionList. (#3247)" (#3349)" This reverts commit a6b41694611ae2eb6d5fcb8928491e8478c4a60d. --- .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 | 51 +++++++------- src/SelectPanel/SelectPanel.test.tsx | 2 +- src/SelectPanel/SelectPanel.tsx | 23 +++--- src/drafts/MarkdownEditor/_SavedReplies.tsx | 14 +++- 12 files changed, 166 insertions(+), 128 deletions(-) create mode 100644 .changeset/weak-jokes-chew.md diff --git a/.changeset/weak-jokes-chew.md b/.changeset/weak-jokes-chew.md new file mode 100644 index 00000000000..1c2445cec20 --- /dev/null +++ b/.changeset/weak-jokes-chew.md @@ -0,0 +1,5 @@ +--- +"@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 0134547dccc..b07b87c8731 100644 --- a/docs/content/SelectPanel.mdx +++ b/docs/content/SelectPanel.mdx @@ -12,20 +12,18 @@ A `SelectPanel` provides an anchor that will open an overlay with a list of sele ```javascript live noinline function getColorCircle(color) { - return function () { - return ( - - ) - } + return ( + + ) } const items = [ diff --git a/generated/components.json b/generated/components.json index 293c88d1410..10fca0f4aed 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 (\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: 'medium',\n height: 'medium',\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 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 9d0c1a8ba7c..f9a779c6f56 100644 --- a/src/ActionList/List.tsx +++ b/src/ActionList/List.tsx @@ -69,7 +69,8 @@ export const List = React.forwardRef( value={{ variant, selectionVariant: selectionVariant || containerSelectionVariant, - showDividers, + // @ts-ignore showItemDividers may be passed by some components until next major. + showDividers: showDividers || !!props.showItemDividers, role: role || listRole, headingId, }} diff --git a/src/FilteredActionList/FilteredActionList.stories.tsx b/src/FilteredActionList/FilteredActionList.stories.tsx index 1b8d938c500..ab1dbcc1b60 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} from '../FilteredActionList' +import {FilteredActionList, ItemInput} from '../FilteredActionList' import BaseStyles from '../BaseStyles' import Box from '../Box' @@ -26,35 +26,33 @@ const meta: Meta = { export default meta function getColorCircle(color: string) { - return function () { - return ( - - ) - } + 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'}, +] as ItemInput[] 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 00fc8ae8bb6..670d29a8d63 100644 --- a/src/FilteredActionList/FilteredActionList.tsx +++ b/src/FilteredActionList/FilteredActionList.tsx @@ -1,33 +1,42 @@ -import type {ScrollIntoViewOptions} from '@primer/behaviors' -import {scrollIntoView} from '@primer/behaviors' import React, {KeyboardEventHandler, useCallback, useEffect, useRef} from 'react' -import styled from 'styled-components' +import TextInput, {TextInputProps} from '../TextInput' import Box from '../Box' +import {ActionList, ActionListProps, ActionListItemProps} from '../ActionList' import Spinner from '../Spinner' -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 styled from 'styled-components' +import {get} from '../constants' +import {useProvidedRefOrCreate} from '../hooks/useProvidedRefOrCreate' 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 interface FilteredActionListProps - extends Partial>, - ListPropsBase, - SxProp { +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 { loading?: boolean placeholderText?: string filterValue?: string onFilterChange: (value: string, e: React.ChangeEvent) => void textInputProps?: Partial> inputRef?: React.RefObject + items: ItemInput[] } const StyledHeader = styled.div` @@ -35,6 +44,27 @@ 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, @@ -57,7 +87,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() @@ -84,7 +114,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) { @@ -132,7 +162,15 @@ export function FilteredActionList({ ) : ( - + + {items.map(i => renderFn(i))} + )} diff --git a/src/FilteredActionList/index.ts b/src/FilteredActionList/index.ts index 3f8176fe71c..8a96b74fd6f 100644 --- a/src/FilteredActionList/index.ts +++ b/src/FilteredActionList/index.ts @@ -1,2 +1,2 @@ export {FilteredActionList} from './FilteredActionList' -export type {FilteredActionListProps} from './FilteredActionList' +export type {FilteredActionListProps, ItemInput} from './FilteredActionList' diff --git a/src/SelectPanel/SelectPanel.features.stories.tsx b/src/SelectPanel/SelectPanel.features.stories.tsx index 42b4f6ccf24..b833a8bb614 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 '../deprecated/ActionList/List' +import {ItemInput} from '../FilteredActionList' import {SelectPanel} from './SelectPanel' import {TriangleDownIcon} from '@primer/octicons-react' import type {OverlayProps} from '../Overlay' @@ -14,30 +14,28 @@ export default { } as ComponentMeta function getColorCircle(color: string) { - return function () { - return ( - - ) - } + 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 = () => { @@ -63,6 +61,7 @@ export const SingleSelectStory = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} + showDividers={true} showItemDividers={true} overlayProps={{width: 'small', height: 'xsmall'}} /> @@ -94,7 +93,6 @@ export const ExternalAnchorStory = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} - showItemDividers={true} overlayProps={{width: 'small', height: 'xsmall'}} /> @@ -125,7 +123,6 @@ export const SelectPanelHeightInitialWithOverflowingItemsStory = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} - showItemDividers={true} overlayProps={{width: 'small', height: 'initial', maxHeight: 'xsmall'}} /> @@ -157,7 +154,6 @@ export const SelectPanelHeightInitialWithUnderflowingItemsStory = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} - showItemDividers={true} overlayProps={{width: 'small', height: 'initial', maxHeight: 'xsmall'}} /> @@ -202,7 +198,6 @@ export const SelectPanelHeightInitialWithUnderflowingItemsAfterFetch = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} - showItemDividers={true} overlayProps={{width: 'small', height, maxHeight: 'xsmall'}} /> @@ -234,7 +229,6 @@ 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

@@ -293,7 +286,6 @@ 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 11206c4943b..a87d6d5c932 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 {ItemInput} from '../deprecated/ActionList/List' +import Box from '../Box' +import {ItemInput} from '../FilteredActionList' export default { title: 'Components/SelectPanel', @@ -13,32 +13,30 @@ export default { } as ComponentMeta function getColorCircle(color: string) { - return function () { - return ( - - ) - } + 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 = () => { @@ -65,8 +63,7 @@ export const Default = () => { selected={selected} onSelectedChange={setSelected} onFilterChange={setFilter} - showItemDividers={true} - overlayProps={{width: 'medium', height: 'medium'}} + overlayProps={{width: 'small', height: 'xsmall'}} /> ) diff --git a/src/SelectPanel/SelectPanel.test.tsx b/src/SelectPanel/SelectPanel.test.tsx index 05332623326..551d742aaa3 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 '../deprecated/ActionList/List' +import {ItemInput} from '../FilteredActionList' expect.extend(toHaveNoViolations) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index 0075069d50d..47da07df34f 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -1,17 +1,16 @@ 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' @@ -44,7 +43,8 @@ export type SelectPanelProps = SelectPanelBaseProps & Omit & Pick & AnchoredOverlayWrapperAnchorProps & - (SelectPanelSingleSelection | SelectPanelMultiSelection) + // TODO: 23-05-23 - Remove showItemDividers after next-major release + (SelectPanelSingleSelection | SelectPanelMultiSelection) & {showItemDividers?: boolean} function isMultiSelectVariant( selected: SelectPanelSingleSelection['selected'] | SelectPanelMultiSelection['selected'], @@ -123,9 +123,7 @@ export function SelectPanel({ ...item, role: 'option', selected: 'selected' in item && item.selected === undefined ? undefined : isItemSelected, - onAction: (itemFromAction, event) => { - item.onAction?.(itemFromAction, event) - + onSelect: (event: React.MouseEvent | React.KeyboardEvent) => { if (event.defaultPrevented) { return } @@ -144,7 +142,7 @@ export function SelectPanel({ singleSelectOnChange(item === selected ? undefined : item) onClose('selection') }, - } as ItemProps + } as ActionListItemProps }) }, [onClose, onSelectedChange, items, selected]) @@ -204,11 +202,10 @@ 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: { @@ -66,6 +71,7 @@ export const SavedRepliesButton = () => { maxWidth: '100%', }, }, + id: i.toString(), }), ) @@ -105,6 +111,12 @@ 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} /> ) : ( <>