From a09b8f3749b770961e07190ac2fd833dabbcab98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 11:10:49 +0000 Subject: [PATCH 01/34] Change deprecated/ActionList semantics --- src/deprecated/ActionList/Divider.tsx | 4 ++-- src/deprecated/ActionList/Group.tsx | 16 ++++++++++------ src/deprecated/ActionList/Item.tsx | 6 +++--- src/deprecated/ActionList/List.tsx | 13 ++++++++----- 4 files changed, 23 insertions(+), 16 deletions(-) diff --git a/src/deprecated/ActionList/Divider.tsx b/src/deprecated/ActionList/Divider.tsx index 48102214ba8..4c6446c473b 100644 --- a/src/deprecated/ActionList/Divider.tsx +++ b/src/deprecated/ActionList/Divider.tsx @@ -2,7 +2,7 @@ import React from 'react' import styled from 'styled-components' import {get} from '../../constants' -export const StyledDivider = styled.div` +export const StyledDivider = styled.li` height: 1px; background: ${get('colors.border.muted')}; margin-top: calc(${get('space.2')} - 1px); @@ -13,7 +13,7 @@ export const StyledDivider = styled.div` * Visually separates `Item`s or `Group`s in an `ActionList`. */ export function Divider(): JSX.Element { - return + return } /** diff --git a/src/deprecated/ActionList/Group.tsx b/src/deprecated/ActionList/Group.tsx index 638ea4abf64..d14eccd81ff 100644 --- a/src/deprecated/ActionList/Group.tsx +++ b/src/deprecated/ActionList/Group.tsx @@ -6,7 +6,7 @@ import {Header, HeaderProps} from './Header' /** * Contract for props passed to the `Group` component. */ -export interface GroupProps extends React.ComponentPropsWithoutRef<'div'>, SxProp { +export interface GroupProps extends React.ComponentPropsWithoutRef<'ul'>, SxProp { /** * Props for a `Header` to render in the `Group`. */ @@ -28,8 +28,10 @@ export interface GroupProps extends React.ComponentPropsWithoutRef<'div'>, SxPro showItemDividers?: boolean } -const StyledGroup = styled.div` +const StyledGroup = styled.ul` ${sx} + list-style: none; + padding-inline-start: 0; ` /** @@ -37,9 +39,11 @@ const StyledGroup = styled.div` */ export function Group({header, items, ...props}: GroupProps): JSX.Element { return ( - - {header &&
} - {items} - +
  • + + {header &&
    } + {items} + +
    • ) } diff --git a/src/deprecated/ActionList/Item.tsx b/src/deprecated/ActionList/Item.tsx index 70f4a3db399..b5868afccdd 100644 --- a/src/deprecated/ActionList/Item.tsx +++ b/src/deprecated/ActionList/Item.tsx @@ -97,7 +97,7 @@ export interface ItemProps extends SxProp { /** * Callback that will trigger both on click selection and keyboard selection. */ - onAction?: (item: ItemProps, event: React.MouseEvent | React.KeyboardEvent) => void + onAction?: (item: ItemProps, event: React.MouseEvent | React.KeyboardEvent) => void /** * An id associated with this item. Should be unique between items @@ -170,7 +170,7 @@ const MainContent = styled.div` flex-grow: 1; ` -const StyledItem = styled.div< +const StyledItem = styled.li< { variant: ItemProps['variant'] showDivider: ItemProps['showDivider'] @@ -476,6 +476,6 @@ export const Item = React.forwardRef((itemProps, ref) => { ) -}) as PolymorphicForwardRefComponent<'div', ItemProps> +}) as PolymorphicForwardRefComponent<'li', ItemProps> Item.displayName = 'ActionList.Item' diff --git a/src/deprecated/ActionList/List.tsx b/src/deprecated/ActionList/List.tsx index 0af5190d7c1..c7423ae33dc 100644 --- a/src/deprecated/ActionList/List.tsx +++ b/src/deprecated/ActionList/List.tsx @@ -12,7 +12,7 @@ import {Merge} from '../../utils/types/Merge' type RenderItemFn = (props: ItemProps) => React.ReactElement export type ItemInput = - | Merge, ItemProps> + | Merge, ItemProps> | ((Partial & {renderItem: RenderItemFn}) & {key?: Key}) /** @@ -100,7 +100,7 @@ function isGroupedListProps(props: ListProps): props is GroupedListProps { */ export type ListProps = ListPropsBase | GroupedListProps -const StyledList = styled.div` +const ListBox = styled.ul` font-size: ${get('fontSizes.1')}; /* 14px font-size * 1.428571429 = 20px line height * @@ -108,6 +108,9 @@ const StyledList = styled.div` * hardcoded '20px' */ line-height: 20px; + padding-inline-start: 0; + margin-block-start: 0; + margin-block-end: 0; &[${hasActiveDescendantAttribute}], &:focus-within { --item-hover-bg-override: none; @@ -143,7 +146,7 @@ function useListVariant(variant: ListProps['variant'] = 'inset'): { /** * Lists `Item`s, either grouped or ungrouped, with a `Divider` between each `Group`. */ -export const List = React.forwardRef((props, forwardedRef): JSX.Element => { +export const List = React.forwardRef((props, forwardedRef): JSX.Element => { // Get `sx` prop values for `List` children matching the given `List` style variation. const {firstGroupStyle, lastGroupStyle, headerStyle, itemStyle} = useListVariant(props.variant) @@ -227,7 +230,7 @@ export const List = React.forwardRef((props, forwarde } return ( - + {groups.map(({header, ...groupProps}, index) => { const hasFilledHeader = header?.variant === 'filled' const shouldShowDivider = index > 0 && !hasFilledHeader @@ -251,7 +254,7 @@ export const List = React.forwardRef((props, forwarde ) })} - + ) }) From 5dceefae47ef6fcf4b49ae2458a10e5c1705cf89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 11:12:44 +0000 Subject: [PATCH 02/34] Remove item selection via Enter key --- src/deprecated/ActionList/Item.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/deprecated/ActionList/Item.tsx b/src/deprecated/ActionList/Item.tsx index b5868afccdd..565e62f7ba1 100644 --- a/src/deprecated/ActionList/Item.tsx +++ b/src/deprecated/ActionList/Item.tsx @@ -362,8 +362,9 @@ export const Item = React.forwardRef((itemProps, ref) => { } onKeyPress?.(event) - if (!event.defaultPrevented && [' ', 'Enter'].includes(event.key)) { + if (!event.defaultPrevented && [' '].includes(event.key)) { onAction?.(itemProps, event) + event.preventDefault() } }, [onAction, disabled, itemProps, onKeyPress] From f00baaeea2a75b805c2cdd0f6858fc294cfe8377 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 11:15:26 +0000 Subject: [PATCH 03/34] Handle focus on deprecated/ActionList --- src/deprecated/ActionList/List.tsx | 57 ++++++++++++++++-------------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/src/deprecated/ActionList/List.tsx b/src/deprecated/ActionList/List.tsx index c7423ae33dc..6afd838d3b9 100644 --- a/src/deprecated/ActionList/List.tsx +++ b/src/deprecated/ActionList/List.tsx @@ -6,8 +6,9 @@ import {Divider} from './Divider' import styled from 'styled-components' import {get} from '../../constants' import {SystemCssProperties} from '@styled-system/css' -import {hasActiveDescendantAttribute} from '@primer/behaviors' +import {FocusKeys, hasActiveDescendantAttribute} from '@primer/behaviors' import {Merge} from '../../utils/types/Merge' +import {useFocusZone} from '../../hooks/useFocusZone' type RenderItemFn = (props: ItemProps) => React.ReactElement @@ -229,32 +230,36 @@ export const List = React.forwardRef((props, forwar groups = [...groupMap.values()] } + const {containerRef} = useFocusZone({bindKeys: FocusKeys.ArrowVertical | FocusKeys.HomeAndEnd | FocusKeys.PageUpDown}) + return ( - - {groups.map(({header, ...groupProps}, index) => { - const hasFilledHeader = header?.variant === 'filled' - const shouldShowDivider = index > 0 && !hasFilledHeader - return ( - - {shouldShowDivider ? : null} - {renderGroup({ - sx: { - ...(index === 0 && firstGroupStyle), - ...(index === groups.length - 1 && lastGroupStyle), - ...(index > 0 && !shouldShowDivider && {mt: 2}) - }, - ...(header && { - header: { - ...header, - sx: {...headerStyle, ...header.sx} - } - }), - ...groupProps - })} - - ) - })} - +
    }> + + {groups.map(({header, ...groupProps}, index) => { + const hasFilledHeader = header?.variant === 'filled' + const shouldShowDivider = index > 0 && !hasFilledHeader + return ( + + {shouldShowDivider ? : null} + {renderGroup({ + sx: { + ...(index === 0 && firstGroupStyle), + ...(index === groups.length - 1 && lastGroupStyle), + ...(index > 0 && !shouldShowDivider && {mt: 2}) + }, + ...(header && { + header: { + ...header, + sx: {...headerStyle, ...header.sx} + } + }), + ...groupProps + })} + + ) + })} + +
    ) }) From 5e0c3aae44ba86d736145652cfdbf6bba20a39ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 11:24:48 +0000 Subject: [PATCH 04/34] Adapt deprecated/ActionMenu prop --- src/deprecated/ActionMenu.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deprecated/ActionMenu.tsx b/src/deprecated/ActionMenu.tsx index 81bbcb824bc..e1c454f7555 100644 --- a/src/deprecated/ActionMenu.tsx +++ b/src/deprecated/ActionMenu.tsx @@ -18,7 +18,7 @@ interface ActionMenuBaseProps extends Partial | React.KeyboardEvent) => void + onAction?: (props: ItemProps, event?: React.MouseEvent | React.KeyboardEvent) => void /** * If defined, will control the open/closed state of the overlay. Must be used in conjuction with `setOpen`. From a7e3b74af529c1621e20048d41a531609b56b65c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 11:25:11 +0000 Subject: [PATCH 05/34] Update deprecated/ActionList tests and stories --- .../__snapshots__/Autocomplete.test.tsx.snap | 4552 +++++++++-------- src/__tests__/deprecated/ActionList.test.tsx | 10 +- .../__snapshots__/ActionList.test.tsx.snap | 28 +- src/stories/deprecated/ActionList.stories.tsx | 82 +- 4 files changed, 2389 insertions(+), 2283 deletions(-) diff --git a/src/__tests__/__snapshots__/Autocomplete.test.tsx.snap b/src/__tests__/__snapshots__/Autocomplete.test.tsx.snap index 43c754dffc5..baf09f32c61 100644 --- a/src/__tests__/__snapshots__/Autocomplete.test.tsx.snap +++ b/src/__tests__/__snapshots__/Autocomplete.test.tsx.snap @@ -500,6 +500,8 @@ Array [ .c2 { margin-top: 8px; margin-bottom: 8px; + list-style: none; + padding-inline-start: 0; } .c8 { @@ -665,6 +667,9 @@ Array [ .c1 { font-size: 14px; line-height: 20px; + padding-inline-start: 0; + margin-block-start: 0; + margin-block-end: 0; } .c1[data-has-active-descendant], @@ -684,629 +689,636 @@ Array [ className="c0" >
    -
    -
    +
      -
      -
      - -
      -
      -
      - - zero - -
      -
      -
      -
      -
      - -
      -
      -
      -
      +
      - one - -
      -
      -
      -
      -
      - + zero + +
      +
      + +
    • - - - -
    -
    -
    - - two - -
    -
    -
    -
    -
    - -
    -
    -
    -
    +
    - three - -
    -
    -
    -
    -
    - -
    -
    -
    + + one + +
    +
    + +
  • - - four - -
    • -
    - -
    -
    - -
    -
    -
    -
    +
    - five - -
    -
    -
    -
    -
    - -
    -
    -
    + + two + +
    +
    + +
  • - - six - -
    • - - -
    -
    - +
    +
    +
    + + three + +
    +
    + +
  • - - - -
    • -
    -
    + +
    +
    +
    + + four + +
    +
    + +
  • - - seven - -
    • - - -
    -
    - +
    +
    +
    + + five + +
    +
    + +
  • - - - -
    • -
    -
    + +
    +
    +
    + + six + +
    +
    + +
  • - - twenty - -
    • - - -
    -
    - +
    +
    +
    + + seven + +
    +
    + +
  • - - - -
    • -
    -
    + +
    +
    +
    + + twenty + +
    +
    + +
  • - - twentyone - -
    • - - -
    -
    -
    -
    - +
    +
    + + twentyone + +
    +
    + +
  • - - Add new item - -
    • -
    - - +
    +
    + + Add new item + +
    +
    + + + + , @@ -1458,6 +1470,8 @@ Array [ .c2 { margin-top: 8px; margin-bottom: 8px; + list-style: none; + padding-inline-start: 0; } .c8 { @@ -1586,6 +1600,9 @@ Array [ .c1 { font-size: 14px; line-height: 20px; + padding-inline-start: 0; + margin-block-start: 0; + margin-block-end: 0; } .c1[data-has-active-descendant], @@ -1605,576 +1622,583 @@ Array [ className="c0" >
    -
    -
    +
      -
      -
      - -
      -
      -
      - - zero - -
      -
      -
      -
      -
      - -
      -
      -
      -
      +
      - one - -
      -
      -
      -
      -
      - -
      -
      -
      + + zero + +
      +
      + +
    • - - two - -
      • -
    -
    -
    -
    - -
    -
    -
    -
    +
    - three - -
    -
    -
    -
    -
    - -
    -
    -
    + + one + +
    +
    + +
  • - - four - -
    • -
    - -
    -
    - -
    -
    -
    -
    +
    - five - -
    -
    -
    -
    -
    - + two + +
    +
    + +
  • - - - - -
    -
    - - six - -
    -
    - -
    -
    - +
    +
    +
    + + three + +
    +
    +
    • +
  • - - - - -
    -
    + +
    +
    +
    + + four + +
    +
    +
    • +
  • - - seven - - - - -
    -
    - +
    +
    +
    + + five + +
    +
    +
    • +
  • - - - - -
    -
    + +
    +
    +
    + + six + +
    +
    +
    • +
  • - - twenty - - - - -
    -
    - +
    +
    +
    + + seven + +
    +
    +
    • +
  • - - - - -
    -
    + +
    +
    +
    + + twenty + +
    +
    +
    • +
  • - + + +
    - twentyone - -
    - - - +
    + + twentyone + +
    + +
    • + + + , @@ -2326,6 +2350,8 @@ Array [ .c2 { margin-top: 8px; margin-bottom: 8px; + list-style: none; + padding-inline-start: 0; } .c8 { @@ -2465,6 +2491,9 @@ Array [ .c1 { font-size: 14px; line-height: 20px; + padding-inline-start: 0; + margin-block-start: 0; + margin-block-end: 0; } .c1[data-has-active-descendant], @@ -2484,576 +2513,583 @@ Array [ className="c0" >
    -
    -
    +
      -
      -
      - -
      -
      -
      - - zero - -
      -
      -
      -
      -
      - -
      -
      -
      -
      +
      - one - -
      -
      -
      -
      -
      - -
      -
      -
      + + zero + +
      +
      + +
    • - - two - -
      • -
    -
    -
    -
    - -
    -
    -
    -
    +
    - three - -
    -
    -
    -
    -
    - -
    -
    -
    + + one + +
    +
    + +
  • - - four - -
    • -
    - -
    -
    - -
    -
    -
    -
    +
    - five - -
    -
    -
    -
    -
    - -
    -
    -
    + + two + +
    +
    + +
  • - - six - -
    • - - -
    -
    - +
    +
    +
    + + three + +
    +
    + +
  • - - - -
    • -
    -
    + +
    +
    +
    + + four + +
    +
    + +
  • - - seven - -
    • - - -
    -
    - +
    +
    +
    + + five + +
    +
    + +
  • - - - -
    • -
    -
    + +
    +
    +
    + + six + +
    +
    + +
  • - - twenty - -
    • - - -
    -
    - +
    +
    +
    + + seven + +
    +
    + +
  • - - - -
    • -
    -
    + +
    +
    +
    + + twenty + +
    +
    + +
  • - + +
    • +
    - twentyone - -
    - - - +
    + + twentyone + +
    + + + + + , @@ -3205,6 +3241,8 @@ Array [ .c2 { margin-top: 8px; margin-bottom: 8px; + list-style: none; + padding-inline-start: 0; } .c6 { @@ -3301,6 +3339,9 @@ Array [ .c1 { font-size: 14px; line-height: 20px; + padding-inline-start: 0; + margin-block-start: 0; + margin-block-end: 0; } .c1[data-has-active-descendant], @@ -3320,306 +3361,313 @@ Array [ className="c0" >
    -
    -
    +
      -
      -
      -
      - - zero - -
      -
      -
      -
      -
      -
      + + zero + +
      +
      + +
    • - - one - -
      • -
    -
    -
    -
    -
    + + one + +
    +
    + +
  • - - two - -
    • -
    - -
    -
    -
    + + two + +
    +
    + +
  • - - three - -
    • - - -
    -
    -
    + + three + +
    +
    + +
  • - - four - -
    • - - -
    -
    -
    + + four + +
    +
    + +
  • - - five - -
    • - - -
    -
    -
    + + five + +
    +
    + +
  • - - six - -
    • - - -
    -
    -
    + + six + +
    +
    + +
  • - - seven - -
    • - - -
    -
    -
    + + seven + +
    +
    + +
  • - - twenty - -
    • - - -
    -
    -
    + + twenty + +
    +
    + +
  • - - twentyone - -
    • - - - +
    + + twentyone + +
    + + + + + , @@ -3648,6 +3696,8 @@ Array [ .c2 { margin-top: 8px; margin-bottom: 8px; + list-style: none; + padding-inline-start: 0; } .c6 { @@ -3744,6 +3794,9 @@ Array [ .c1 { font-size: 14px; line-height: 20px; + padding-inline-start: 0; + margin-block-start: 0; + margin-block-end: 0; } .c1[data-has-active-descendant], @@ -3763,306 +3816,313 @@ Array [ className="c0" >
    -
    -
    +
      -
      -
      -
      - - zero - -
      -
      -
      -
      -
      -
      + + zero + +
      +
      + +
    • - - one - -
      • -
    -
    -
    -
    -
    + + one + +
    +
    + +
  • - - two - -
    • -
    - -
    -
    -
    + + two + +
    +
    + +
  • - - three - -
    • - - -
    -
    -
    + + three + +
    +
    + +
  • - - four - -
    • - - -
    -
    -
    + + four + +
    +
    + +
  • - - five - -
    • - - -
    -
    -
    + + five + +
    +
    + +
  • - - six - -
    • - - -
    -
    -
    + + six + +
    +
    + +
  • - - seven - -
    • - - -
    -
    -
    + + seven + +
    +
    + +
  • - - twenty - -
    • - - -
    -
    -
    + + twenty + +
    +
    + +
  • - - twentyone - -
    • - - - +
    + + twentyone + +
    + + + + + , diff --git a/src/__tests__/deprecated/ActionList.test.tsx b/src/__tests__/deprecated/ActionList.test.tsx index 7432588b70a..405053d49fa 100644 --- a/src/__tests__/deprecated/ActionList.test.tsx +++ b/src/__tests__/deprecated/ActionList.test.tsx @@ -13,12 +13,14 @@ function SimpleActionList(): JSX.Element { diff --git a/src/__tests__/deprecated/__snapshots__/ActionList.test.tsx.snap b/src/__tests__/deprecated/__snapshots__/ActionList.test.tsx.snap index 852201546b0..ecae35fee20 100644 --- a/src/__tests__/deprecated/__snapshots__/ActionList.test.tsx.snap +++ b/src/__tests__/deprecated/__snapshots__/ActionList.test.tsx.snap @@ -4,11 +4,16 @@ exports[`ActionList renders consistently 1`] = ` .c1 { margin-top: 8px; margin-bottom: 8px; + list-style: none; + padding-inline-start: 0; } .c0 { font-size: 14px; line-height: 20px; + padding-inline-start: 0; + margin-block-start: 0; + margin-block-end: 0; } .c0[data-has-active-descendant], @@ -17,12 +22,19 @@ exports[`ActionList renders consistently 1`] = ` --item-hover-divider-border-color-override: hsla(210,18%,87%,1); } -
    -
    +
    +
      +
    • +
        + +
    `; @@ -201,7 +213,7 @@ exports[`ActionList.Item renders consistently 1`] = ` } } -
    -
    + `; diff --git a/src/stories/deprecated/ActionList.stories.tsx b/src/stories/deprecated/ActionList.stories.tsx index 0659ae39d9c..2148cd4c288 100644 --- a/src/stories/deprecated/ActionList.stories.tsx +++ b/src/stories/deprecated/ActionList.stories.tsx @@ -57,6 +57,7 @@ export function ActionsStory(): JSX.Element {

    Actions

    @@ -86,12 +89,13 @@ export function SimpleListStory(): JSX.Element {

    Simple List

    @@ -113,9 +117,11 @@ export function SingleSelectListStory(): JSX.Element {

    Single Select List

    ({ ...item, - selected: index === 1 + selected: index === 1, + role: 'option' }))} /> @@ -130,10 +136,12 @@ export function MultiSelectListStory(): JSX.Element {

    Multi Select List

    ({ ...item, - selected: index === 1 || index === 3 + selected: index === 1 || index === 3, + role: 'option' }))} /> @@ -152,6 +160,7 @@ export function ComplexListInsetVariantStory(): JSX.Element {

    Inset Variant

    }, { @@ -194,22 +205,25 @@ export function ComplexListInsetVariantStory(): JSX.Element { text: 'Table', description: 'Information-dense table optimized for operations across teams', descriptionVariant: 'block', - groupId: '2' + groupId: '2', + role: 'option' }, { leadingVisual: ProjectIcon, text: 'Board', description: 'Kanban-style board focused on visual states', descriptionVariant: 'block', - groupId: '2' + groupId: '2', + role: 'option' }, { leadingVisual: FilterIcon, text: 'Save sort and filters to current view', - groupId: '3' + groupId: '3', + role: 'option' }, - {leadingVisual: FilterIcon, text: 'Save sort and filters to new view', groupId: '3'}, - {leadingVisual: GearIcon, text: 'View settings', groupId: '4'} + {leadingVisual: FilterIcon, text: 'Save sort and filters to new view', groupId: '3', role: 'option'}, + {leadingVisual: GearIcon, text: 'View settings', groupId: '4', role: 'option'} ]} /> @@ -228,6 +242,7 @@ export function ComplexListFullVariantStory(): JSX.Element {

    Full Variant

    }, { @@ -265,22 +281,25 @@ export function ComplexListFullVariantStory(): JSX.Element { text: 'Table', description: 'Information-dense table optimized for operations across teams', descriptionVariant: 'block', - groupId: '2' + groupId: '2', + role: 'option' }, { leadingVisual: ProjectIcon, text: 'Board', description: 'Kanban-style board focused on visual states', descriptionVariant: 'block', - groupId: '2' + groupId: '2', + role: 'option' }, { leadingVisual: FilterIcon, text: 'Save sort and filters to current view', - groupId: '3' + groupId: '3', + role: 'option' }, - {leadingVisual: FilterIcon, text: 'Save sort and filters to new view', groupId: '3'}, - {leadingVisual: GearIcon, text: 'View settings', groupId: '4'} + {leadingVisual: FilterIcon, text: 'Save sort and filters to new view', groupId: '3', role: 'option'}, + {leadingVisual: GearIcon, text: 'View settings', groupId: '4', role: 'option'} ]} /> @@ -308,11 +327,13 @@ export function CustomItemChildren(): JSX.Element {

    Custom Item Children

    Choose this one, - trailingIcon: ArrowLeftIcon + trailingIcon: ArrowLeftIcon, + role: 'option' } ]} /> @@ -328,6 +349,7 @@ export function SizeStressTestingStory(): JSX.Element {

    Size Stress Testing

    @@ -383,21 +408,26 @@ export function LinkItemStory(): JSX.Element {

    Simple List

    alert('hi?')} {...props} /> }, { text: 'B. Vanilla link', + role: 'option', renderItem: props => }, { text: 'C. React Router link', + role: 'option', renderItem: props => }, { text: 'D. NextJS style', + role: 'option', renderItem: props => ( @@ -418,9 +448,11 @@ export function DOMPropsStory(): JSX.Element {

    Simple List

    alert('Hello') } ]} From bc30a7b5a68959fb4cc4f34e39641dec7df4439e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 11:28:27 +0000 Subject: [PATCH 06/34] Update deprecated/ActionList docs --- docs/content/deprecated/ActionList.mdx | 52 +++++++++++++++----------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/docs/content/deprecated/ActionList.mdx b/docs/content/deprecated/ActionList.mdx index c25de07a382..4768212314a 100644 --- a/docs/content/deprecated/ActionList.mdx +++ b/docs/content/deprecated/ActionList.mdx @@ -14,12 +14,13 @@ Use [new version of ActionList](/ActionList) with composable API, design updates ```jsx ``` @@ -27,12 +28,12 @@ Use [new version of ActionList](/ActionList) with composable API, design updates **After** ```jsx - - New file - Copy link - Edit file + + New file + Copy link + Edit file - Delete file + Delete file ``` @@ -46,12 +47,13 @@ import {ActionList} from '@primer/react/deprecated' ```jsx live deprecated ``` @@ -60,6 +62,7 @@ import {ActionList} from '@primer/react/deprecated' ```jsx live deprecated ``` @@ -104,17 +110,21 @@ import {ActionList} from '@primer/react/deprecated' ```jsx deprecated }, { text: 'React Router link', + role: 'option', renderItem: props => }, { text: 'NextJS style', + role: 'option', renderItem: props => ( From 1fa7658c895327b95501cf5a7319592d3ce2cdb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 11:34:43 +0000 Subject: [PATCH 07/34] Remove focusZone from the FilteredActionList component --- src/FilteredActionList/FilteredActionList.tsx | 26 +------------------ 1 file changed, 1 insertion(+), 25 deletions(-) diff --git a/src/FilteredActionList/FilteredActionList.tsx b/src/FilteredActionList/FilteredActionList.tsx index 665cf6a9c12..0c41f5a1b65 100644 --- a/src/FilteredActionList/FilteredActionList.tsx +++ b/src/FilteredActionList/FilteredActionList.tsx @@ -5,7 +5,6 @@ import TextInput, {TextInputProps} from '../TextInput' import Box from '../Box' import {ActionList} from '../deprecated/ActionList' import Spinner from '../Spinner' -import {useFocusZone} from '../hooks/useFocusZone' import {useProvidedStateOrCreate} from '../hooks/useProvidedStateOrCreate' import styled from 'styled-components' import {get} from '../constants' @@ -56,7 +55,6 @@ export function FilteredActionList({ ) const scrollContainerRef = useRef(null) - const listContainerRef = useRef(null) const inputRef = useProvidedRefOrCreate(providedInputRef) const activeDescendantRef = useRef() const listId = useSSRSafeId() @@ -74,28 +72,6 @@ export function FilteredActionList({ [activeDescendantRef] ) - useFocusZone( - { - containerRef: listContainerRef, - focusOutBehavior: 'wrap', - focusableElementFilter: element => { - return !(element instanceof HTMLInputElement) - }, - activeDescendantFocus: inputRef, - onActiveDescendantChanged: (current, previous, directlyActivated) => { - activeDescendantRef.current = current - - if (current && scrollContainerRef.current && directlyActivated) { - scrollIntoView(current, scrollContainerRef.current, menuScrollMargins) - } - } - }, - [ - // List ref isn't set while loading. Need to re-bind focus zone when it changes - loading - ] - ) - useEffect(() => { // if items changed, we want to instantly move active descendant into view if (activeDescendantRef.current && scrollContainerRef.current) { @@ -128,7 +104,7 @@ export function FilteredActionList({ ) : ( - + )} From 2ab0a6c806a82a1e32d0e400017fe89b1e9b45ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 11:36:50 +0000 Subject: [PATCH 08/34] Add sr-only description for input in the FilteredActionList component --- src/FilteredActionList/FilteredActionList.tsx | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/FilteredActionList/FilteredActionList.tsx b/src/FilteredActionList/FilteredActionList.tsx index 0c41f5a1b65..84dd513fdec 100644 --- a/src/FilteredActionList/FilteredActionList.tsx +++ b/src/FilteredActionList/FilteredActionList.tsx @@ -21,7 +21,7 @@ export interface FilteredActionListProps ListPropsBase, SxProp { loading?: boolean - placeholderText: string + placeholderText?: string filterValue?: string onFilterChange: (value: string, e: React.ChangeEvent) => void textInputProps?: Partial> @@ -33,6 +33,18 @@ const StyledHeader = styled.div` z-index: 1; ` +// sr-only +const SrOnly = styled.span` + position: absolute; + width: 1px; + height: 1px; + padding: 0; + margin: -1px; + overflow: hidden; + clip: rect(0, 0, 0, 0); + border: 0; +` + export function FilteredActionList({ loading = false, placeholderText, @@ -58,6 +70,7 @@ export function FilteredActionList({ const inputRef = useProvidedRefOrCreate(providedInputRef) const activeDescendantRef = useRef() const listId = useSSRSafeId() + const inputDescriptionTextId = useSSRSafeId() const onInputKeyPress: KeyboardEventHandler = useCallback( event => { if (event.key === 'Enter' && activeDescendantRef.current) { @@ -95,8 +108,10 @@ export function FilteredActionList({ placeholder={placeholderText} aria-label={placeholderText} aria-controls={listId} + aria-describedby={inputDescriptionTextId} {...textInputProps} /> + Items will be filtered as you type {loading ? ( From 19afe427b852728c2674ae05f72c46c95ee67309 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 11:57:19 +0000 Subject: [PATCH 09/34] Adapt SelectPanel: semantics, visual layout, add title, close button, save/cancel button and functionality. Update tests, docs, and stories. --- docs/content/SelectPanel.mdx | 3 +- src/SelectPanel/SelectPanel.tsx | 165 +++++++++++++++++------ src/__tests__/SelectPanel.test.tsx | 3 +- src/__tests__/SelectPanel.types.test.tsx | 6 +- src/stories/SelectPanel.stories.tsx | 134 +++++++----------- 5 files changed, 184 insertions(+), 127 deletions(-) diff --git a/docs/content/SelectPanel.mdx b/docs/content/SelectPanel.mdx index a044fc2af21..94d51d9ba12 100644 --- a/docs/content/SelectPanel.mdx +++ b/docs/content/SelectPanel.mdx @@ -49,7 +49,8 @@ function DemoComponent() { {children || 'Select Labels'} )} - placeholderText="Filter Labels" + title="Select Items" + inputLabel="Select Items" open={open} onOpenChange={setOpen} items={filteredItems} diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index 7226bc63126..e2458f9ef2a 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -1,4 +1,4 @@ -import React, {useCallback, useMemo} from 'react' +import React, {useCallback, useMemo, useState} from 'react' import {FilteredActionList, FilteredActionListProps} from '../FilteredActionList' import {OverlayProps} from '../Overlay' import {ItemInput} from '../deprecated/ActionList/List' @@ -10,6 +10,13 @@ import {TextInputProps} from '../TextInput' import {useProvidedStateOrCreate} from '../hooks/useProvidedStateOrCreate' import {AnchoredOverlayWrapperAnchorProps} from '../AnchoredOverlay/AnchoredOverlay' import {useProvidedRefOrCreate} from '../hooks' +import styled from 'styled-components' +import {Button, IconButton} from '../Button' +import {SearchIcon, XIcon} from '@primer/octicons-react' +import sx, {SxProp} from '../sx' +import {get} from '../constants' +import Box from '../Box' +import {useSSRSafeId} from '@react-aria/ssr' interface SelectPanelSingleSelection { selected: ItemInput | undefined @@ -26,7 +33,8 @@ interface SelectPanelBaseProps { open: boolean, gesture: 'anchor-click' | 'anchor-key-press' | 'click-outside' | 'escape' | 'selection' ) => void - placeholder?: string + title: string + inputLabel: string overlayProps?: Partial } @@ -47,12 +55,32 @@ const focusZoneSettings: Partial = { disabled: true } +// sr-only +const SrOnly = styled.span` + position: absolute; + width: 1px; + height: 1px; + padding: 0; + margin: -1px; + overflow: hidden; + clip: rect(0, 0, 0, 0); + border: 0; +` + +const Title = styled.h1` + font-size: ${get('fontSizes.1')}; + font-weight: ${get('fontWeights.bold')}; + margin: 0; /* override default margin */ + ${sx}; +` + export function SelectPanel({ open, onOpenChange, renderAnchor = props => , anchorRef: externalAnchorRef, - placeholder, + title, + inputLabel, selected, onSelectedChange, filterValue: externalFilterValue, @@ -60,7 +88,7 @@ export function SelectPanel({ items, textInputProps, overlayProps, - sx, + sx: sxProp, ...listProps }: SelectPanelProps): JSX.Element { const [filterValue, setInternalFilterValue] = useProvidedStateOrCreate(externalFilterValue, undefined, '') @@ -72,6 +100,13 @@ export function SelectPanel({ [externalOnFilterChange, setInternalFilterValue] ) + const selectedItems = React.useMemo( + () => (Array.isArray(selected) ? selected : [...(selected ? [selected] : [])]), + [selected] + ) + + const [finalItemsSelected, setFinalItemsSelected] = useState(selectedItems) + const anchorRef = useProvidedRefOrCreate(externalAnchorRef) const onOpen: AnchoredOverlayProps['onOpen'] = useCallback(gesture => onOpenChange(true, gesture), [onOpenChange]) const onClose = useCallback( @@ -86,24 +121,20 @@ export function SelectPanel({ return null } - const selectedItems = Array.isArray(selected) ? selected : [...(selected ? [selected] : [])] - return >(props: T) => { return renderAnchor({ ...props, - children: selectedItems.length ? selectedItems.map(item => item.text).join(', ') : placeholder + children: selectedItems.length ? selectedItems.map(item => item.text).join(', ') : inputLabel }) } - }, [placeholder, renderAnchor, selected]) + }, [inputLabel, renderAnchor, selectedItems]) const itemsToRender = useMemo(() => { return items.map(item => { - const isItemSelected = isMultiSelectVariant(selected) ? selected.includes(item) : selected === item - return { ...item, role: 'option', - selected: 'selected' in item && item.selected === undefined ? undefined : isItemSelected, + selected: 'selected' in item && item.selected === undefined ? undefined : finalItemsSelected.includes(item), onAction: (itemFromAction, event) => { item.onAction?.(itemFromAction, event) @@ -112,24 +143,42 @@ export function SelectPanel({ } if (isMultiSelectVariant(selected)) { - const otherSelectedItems = selected.filter(selectedItem => selectedItem !== item) - const newSelectedItems = selected.includes(item) ? otherSelectedItems : [...otherSelectedItems, item] + const otherSelectedItems = finalItemsSelected.filter(selectedItem => selectedItem !== item) + const newSelectedItems = finalItemsSelected.includes(item) + ? otherSelectedItems + : [...otherSelectedItems, item] - const multiSelectOnChange = onSelectedChange as SelectPanelMultiSelection['onSelectedChange'] - multiSelectOnChange(newSelectedItems) - return + setFinalItemsSelected(newSelectedItems) + } else { + // single select + setFinalItemsSelected(finalItemsSelected.includes(item) ? [] : [item]) } - - // single select - const singleSelectOnChange = onSelectedChange as SelectPanelSingleSelection['onSelectedChange'] - singleSelectOnChange(item === selected ? undefined : item) - onClose('selection') } } as ItemProps }) - }, [onClose, onSelectedChange, items, selected]) + }, [items, selected, setFinalItemsSelected, finalItemsSelected]) + + // eslint-disable-next-line no-console + React.useEffect(() => console.debug({finalItemsSelected}), [finalItemsSelected]) + + const onSaveClickHandler = React.useCallback(() => { + if (isMultiSelectVariant(selected)) { + const multiSelectOnChange = onSelectedChange as SelectPanelMultiSelection['onSelectedChange'] + multiSelectOnChange(finalItemsSelected) + } else { + const singleSelectOnChange = onSelectedChange as SelectPanelSingleSelection['onSelectedChange'] + singleSelectOnChange(finalItemsSelected.length > 0 ? finalItemsSelected[0] : undefined) + } + onClose('selection') + }, [finalItemsSelected, onSelectedChange, onClose, selected]) + + const onCancelClickHandler = React.useCallback(() => { + setFinalItemsSelected(selectedItems) + onClose('escape') + }, [onClose, selectedItems]) const inputRef = React.useRef(null) + const titleId = useSSRSafeId() const focusTrapSettings = { initialFocusRef: inputRef } @@ -138,9 +187,21 @@ export function SelectPanel({ return { sx: {m: 2}, contrast: true, + leadingVisual: SearchIcon, + 'aria-label': inputLabel, ...textInputProps } - }, [textInputProps]) + }, [textInputProps, inputLabel]) + + const overlayKeyPressHandler = useCallback( + event => { + if (!event.defaultPrevented && ['Enter'].includes(event.key)) { + onSaveClickHandler() + event.preventDefault() + } + }, + [onSaveClickHandler] + ) return ( - + {/* inheriting height and maxHeight ensures that the content is never taller than the Overlay (which would break scrolling the items) */} + + + {filterValue === '' + ? 'Showing all items' + : items.length <= 0 + ? 'No matching items' + : `${items.length} matching ${items.length === 1 ? 'item' : 'items'}`} + + + {title} + + + + + + + + ) } diff --git a/src/__tests__/SelectPanel.test.tsx b/src/__tests__/SelectPanel.test.tsx index b88cb8f3149..75d396f22ff 100644 --- a/src/__tests__/SelectPanel.test.tsx +++ b/src/__tests__/SelectPanel.test.tsx @@ -23,7 +23,8 @@ function SimpleSelectPanel(): JSX.Element { null} items={[]} @@ -18,7 +19,8 @@ export function shouldAcceptCallWithNoProps() { export function shouldAcceptDOMPropsOnOverlay() { return ( null} items={[]} diff --git a/src/stories/SelectPanel.stories.tsx b/src/stories/SelectPanel.stories.tsx index 8c6ced294ce..30f86176e14 100644 --- a/src/stories/SelectPanel.stories.tsx +++ b/src/stories/SelectPanel.stories.tsx @@ -1,12 +1,11 @@ -import type {OverlayProps} from '../Overlay' import {Meta} from '@storybook/react' import React, {useRef, useState} from 'react' import {theme, ThemeProvider} from '..' -import {ItemInput} from '../deprecated/ActionList/List' import BaseStyles from '../BaseStyles' +import Box from '../Box' +import {ItemInput} from '../deprecated/ActionList/List' import {DropdownButton} from '../deprecated/DropdownMenu' import {SelectPanel} from '../SelectPanel' -import Box from '../Box' const meta: Meta = { title: 'Composite components/SelectPanel', @@ -55,6 +54,20 @@ const items = [ {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: '1-enhancement', id: 21}, + {leadingVisual: getColorCircle('#d73a4a'), text: '1-bug', id: 22}, + {leadingVisual: getColorCircle('#0cf478'), text: '1-good first issue', id: 23}, + {leadingVisual: getColorCircle('#ffd78e'), text: '1-design', id: 24}, + {leadingVisual: getColorCircle('#ff0000'), text: '1-blocker', id: 25}, + {leadingVisual: getColorCircle('#a4f287'), text: '1-backend', id: 26}, + {leadingVisual: getColorCircle('#8dc6fc'), text: '1-frontend', id: 27}, + {leadingVisual: getColorCircle('#a2eeef'), text: '2-enhancement', id: 31}, + {leadingVisual: getColorCircle('#d73a4a'), text: '2-bug', id: 32}, + {leadingVisual: getColorCircle('#0cf478'), text: '2-good first issue', id: 33}, + {leadingVisual: getColorCircle('#ffd78e'), text: '2-design', id: 34}, + {leadingVisual: getColorCircle('#ff0000'), text: '2-blocker', id: 35}, + {leadingVisual: getColorCircle('#a4f287'), text: '2-backend', id: 36}, + {leadingVisual: getColorCircle('#8dc6fc'), text: '2-frontend', id: 37} */ ] export function MultiSelectStory(): JSX.Element { @@ -69,11 +82,12 @@ export function MultiSelectStory(): JSX.Element {
    Please select labels that describe your issue:
    ( - + {children ?? 'Select Labels'} )} - placeholderText="Filter Labels" + title="Select Labels" + inputLabel="Filter Labels" open={open} onOpenChange={setOpen} items={filteredItems} @@ -81,7 +95,7 @@ export function MultiSelectStory(): JSX.Element { onSelectedChange={setSelected} onFilterChange={setFilter} showItemDividers={true} - overlayProps={{width: 'small', height: 'xsmall'}} + overlayProps={{width: 'medium', height: 'large'}} /> ) @@ -100,11 +114,12 @@ export function SingleSelectStory(): JSX.Element {
    Please select a label that describe your issue:
    ( - + {children ?? 'Select Labels'} )} - placeholderText="Filter Labels" + title="Select a Label" + inputLabel="Filter Labels" open={open} onOpenChange={setOpen} items={filteredItems} @@ -112,7 +127,7 @@ export function SingleSelectStory(): JSX.Element { onSelectedChange={setSelected} onFilterChange={setFilter} showItemDividers={true} - overlayProps={{width: 'small', height: 'xsmall'}} + overlayProps={{width: 'medium', height: 'large'}} /> ) @@ -135,7 +150,8 @@ export function ExternalAnchorStory(): JSX.Element { ) } ExternalAnchorStory.storyName = 'With External Anchor' -export function SelectPanelHeightInitialWithOverflowingItemsStory(): JSX.Element { +export function SelectPanelWithOverflowingItemsStory(): JSX.Element { const [selected, setSelected] = React.useState(items[0]) const [filter, setFilter] = React.useState('') const filteredItems = items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) @@ -162,11 +178,12 @@ export function SelectPanelHeightInitialWithOverflowingItemsStory(): JSX.Element
    Please select a label that describe your issue:
    ( - + {children ?? 'Select Labels'} )} - placeholderText="Filter Labels" + title="Select a Label" + inputLabel="Filter Labels" open={open} onOpenChange={setOpen} items={filteredItems} @@ -174,14 +191,14 @@ export function SelectPanelHeightInitialWithOverflowingItemsStory(): JSX.Element onSelectedChange={setSelected} onFilterChange={setFilter} showItemDividers={true} - overlayProps={{width: 'small', height: 'initial', maxHeight: 'xsmall'}} + overlayProps={{width: 'medium', height: 'medium'}} /> ) } -SelectPanelHeightInitialWithOverflowingItemsStory.storyName = 'SelectPanel, Height: Initial, Overflowing Items' +SelectPanelWithOverflowingItemsStory.storyName = 'SelectPanel, Overflowing Items' -export function SelectPanelHeightInitialWithUnderflowingItemsStory(): JSX.Element { +export function SelectPanelWithUnderflowingItemsStory(): JSX.Element { const underflowingItems = [items[0], items[1]] const [selected, setSelected] = React.useState(underflowingItems[0]) const [filter, setFilter] = React.useState('') @@ -194,11 +211,12 @@ export function SelectPanelHeightInitialWithUnderflowingItemsStory(): JSX.Elemen
    Please select a label that describe your issue:
    ( - + {children ?? 'Select Labels'} )} - placeholderText="Filter Labels" + title="Select a Label" + inputLabel="Filter Labels" open={open} onOpenChange={setOpen} items={filteredItems} @@ -206,14 +224,14 @@ export function SelectPanelHeightInitialWithUnderflowingItemsStory(): JSX.Elemen onSelectedChange={setSelected} onFilterChange={setFilter} showItemDividers={true} - overlayProps={{width: 'small', height: 'initial', maxHeight: 'xsmall'}} + overlayProps={{width: 'medium', height: 'medium'}} /> ) } -SelectPanelHeightInitialWithUnderflowingItemsStory.storyName = 'SelectPanel, Height: Initial, Underflowing Items' +SelectPanelWithUnderflowingItemsStory.storyName = 'SelectPanel, Underflowing Items' -export function SelectPanelHeightInitialWithUnderflowingItemsAfterFetch(): JSX.Element { +export function SelectPanelWithUnderflowingItemsAfterFetch(): JSX.Element { const [selected, setSelected] = React.useState(items[0]) const [filter, setFilter] = React.useState('') const [fetchedItems, setFetchedItems] = useState([]) @@ -222,13 +240,11 @@ export function SelectPanelHeightInitialWithUnderflowingItemsAfterFetch(): JSX.E [fetchedItems, filter] ) const [open, setOpen] = useState(false) - const [height, setHeight] = useState('auto') const onOpenChange = () => { setOpen(!open) setTimeout(() => { setFetchedItems([items[0], items[1]]) - setHeight('initial') }, 1500) } @@ -238,11 +254,12 @@ export function SelectPanelHeightInitialWithUnderflowingItemsAfterFetch(): JSX.E
    Please select a label that describe your issue:
    ( - + {children ?? 'Select Labels'} )} - placeholderText="Filter Labels" + title="Select a Label" + inputLabel="Filter Labels" open={open} onOpenChange={onOpenChange} loading={filteredItems.length === 0} @@ -251,13 +268,12 @@ export function SelectPanelHeightInitialWithUnderflowingItemsAfterFetch(): JSX.E onSelectedChange={setSelected} onFilterChange={setFilter} showItemDividers={true} - overlayProps={{width: 'small', height, maxHeight: 'xsmall'}} + overlayProps={{width: 'medium', height: 'medium'}} /> ) } -SelectPanelHeightInitialWithUnderflowingItemsAfterFetch.storyName = - 'SelectPanel, Height: Initial, Underflowing Items (After Fetch)' +SelectPanelWithUnderflowingItemsAfterFetch.storyName = 'SelectPanel, Underflowing Items (After Fetch)' export function SelectPanelAboveTallBody(): JSX.Element { const [selected, setSelected] = React.useState(items[0]) @@ -271,11 +287,12 @@ export function SelectPanelAboveTallBody(): JSX.Element {
    Please select a label that describe your issue:
    ( - + {children ?? 'Select Labels'} )} - placeholderText="Filter Labels" + title="Select a Label" + inputLabel="Filter Labels" open={open} onOpenChange={setOpen} items={filteredItems} @@ -283,7 +300,7 @@ export function SelectPanelAboveTallBody(): JSX.Element { onSelectedChange={setSelected} onFilterChange={setFilter} showItemDividers={true} - overlayProps={{width: 'small', height: 'xsmall'}} + overlayProps={{width: 'medium', height: 'medium'}} />
    (longItems[0]) - const [selectedB, setSelectedB] = React.useState(longItems[0]) - const [filter, setFilter] = React.useState('') - const filteredItems = longItems.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) - const [openA, setOpenA] = useState(false) - const [openB, setOpenB] = useState(false) - - return ( - <> -

    With height:medium

    - ( - - {children ?? 'Select Labels'} - - )} - placeholderText="Filter Labels" - open={openA} - onOpenChange={setOpenA} - items={filteredItems} - selected={selectedA} - onSelectedChange={setSelectedA} - onFilterChange={setFilter} - showItemDividers={true} - overlayProps={{height: 'medium'}} - /> -

    With height:auto, maxheight:medium

    - ( - - {children ?? 'Select Labels'} - - )} - placeholderText="Filter Labels" - open={openB} - onOpenChange={setOpenB} - items={filteredItems} - selected={selectedB} - onSelectedChange={setSelectedB} - onFilterChange={setFilter} - showItemDividers={true} - overlayProps={{ - height: 'auto', - maxHeight: 'medium' - }} - /> - - ) -} -SelectPanelHeightAndScroll.storyName = 'SelectPanel, Height and Scroll' From 94b7ef0600a92b5d92de63c9055c6e01c2a60ece Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 12:00:22 +0000 Subject: [PATCH 10/34] Update SelectPanel test snap --- src/__tests__/__snapshots__/SelectPanel.test.tsx.snap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/__tests__/__snapshots__/SelectPanel.test.tsx.snap b/src/__tests__/__snapshots__/SelectPanel.test.tsx.snap index 17e80b889de..d5516d2f463 100644 --- a/src/__tests__/__snapshots__/SelectPanel.test.tsx.snap +++ b/src/__tests__/__snapshots__/SelectPanel.test.tsx.snap @@ -87,7 +87,7 @@ exports[`SelectPanel renders consistently 1`] = ` aria-expanded={false} aria-haspopup="true" className="c1" - id="react-aria-1" + id="react-aria-2" onClick={[Function]} onKeyDown={[Function]} tabIndex={0} From d0d1eca90026a632ba63190f6aadb87aa8737a67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 13:54:46 +0000 Subject: [PATCH 11/34] Update deprecated/ActionList axe test --- src/__tests__/deprecated/ActionList.test.tsx | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/__tests__/deprecated/ActionList.test.tsx b/src/__tests__/deprecated/ActionList.test.tsx index 405053d49fa..b3f521d24e8 100644 --- a/src/__tests__/deprecated/ActionList.test.tsx +++ b/src/__tests__/deprecated/ActionList.test.tsx @@ -42,7 +42,24 @@ describe('ActionList', () => { it('should have no axe violations', async () => { const {container} = HTMLRender() - const results = await axe(container) + /** + * Axe is throwing an issue because there are
      without
    • as direct children + * This is because we have a group structure inside the listbox: + *
        + *
      • + *
          + *
        • Option 1
          • + *
        + *
        • + *
      + * We have consulted and agreed with our a11y consultant (@jscholes) about this solution. + */ + const results = await axe(container, { + rules: { + 'aria-required-parent': {enabled: false}, + 'aria-required-children': {enabled: false} + } + }) expect(results).toHaveNoViolations() cleanup() }) From f851e04fc914815edb491e558c90baa3470a244e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 13:54:46 +0000 Subject: [PATCH 12/34] Update deprecated/ActionList axe test --- src/SelectPanel/SelectPanel.tsx | 2 +- src/__tests__/deprecated/ActionList.test.tsx | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index e2458f9ef2a..5319361e094 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -210,7 +210,7 @@ export function SelectPanel({ open={open} onOpen={onOpen} onClose={onClose} - overlayProps={{...overlayProps, onKeyPress: overlayKeyPressHandler, role: 'dialog'}} + overlayProps={{...overlayProps, onKeyPress: overlayKeyPressHandler, role: 'dialog', 'aria-labelledby': titleId}} focusTrapSettings={focusTrapSettings} focusZoneSettings={focusZoneSettings} > diff --git a/src/__tests__/deprecated/ActionList.test.tsx b/src/__tests__/deprecated/ActionList.test.tsx index 405053d49fa..b3f521d24e8 100644 --- a/src/__tests__/deprecated/ActionList.test.tsx +++ b/src/__tests__/deprecated/ActionList.test.tsx @@ -42,7 +42,24 @@ describe('ActionList', () => { it('should have no axe violations', async () => { const {container} = HTMLRender() - const results = await axe(container) + /** + * Axe is throwing an issue because there are
        without
      • as direct children + * This is because we have a group structure inside the listbox: + *
          + *
        • + *
            + *
          • Option 1
            • + *
          + *
          • + *
        + * We have consulted and agreed with our a11y consultant (@jscholes) about this solution. + */ + const results = await axe(container, { + rules: { + 'aria-required-parent': {enabled: false}, + 'aria-required-children': {enabled: false} + } + }) expect(results).toHaveNoViolations() cleanup() }) From dfd809661c2bb0e49164e9b455a6964187b8db19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 14:25:41 +0000 Subject: [PATCH 13/34] Delete commented code --- src/stories/SelectPanel.stories.tsx | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/stories/SelectPanel.stories.tsx b/src/stories/SelectPanel.stories.tsx index 30f86176e14..2f85cae7f4a 100644 --- a/src/stories/SelectPanel.stories.tsx +++ b/src/stories/SelectPanel.stories.tsx @@ -54,20 +54,6 @@ const items = [ {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: '1-enhancement', id: 21}, - {leadingVisual: getColorCircle('#d73a4a'), text: '1-bug', id: 22}, - {leadingVisual: getColorCircle('#0cf478'), text: '1-good first issue', id: 23}, - {leadingVisual: getColorCircle('#ffd78e'), text: '1-design', id: 24}, - {leadingVisual: getColorCircle('#ff0000'), text: '1-blocker', id: 25}, - {leadingVisual: getColorCircle('#a4f287'), text: '1-backend', id: 26}, - {leadingVisual: getColorCircle('#8dc6fc'), text: '1-frontend', id: 27}, - {leadingVisual: getColorCircle('#a2eeef'), text: '2-enhancement', id: 31}, - {leadingVisual: getColorCircle('#d73a4a'), text: '2-bug', id: 32}, - {leadingVisual: getColorCircle('#0cf478'), text: '2-good first issue', id: 33}, - {leadingVisual: getColorCircle('#ffd78e'), text: '2-design', id: 34}, - {leadingVisual: getColorCircle('#ff0000'), text: '2-blocker', id: 35}, - {leadingVisual: getColorCircle('#a4f287'), text: '2-backend', id: 36}, - {leadingVisual: getColorCircle('#8dc6fc'), text: '2-frontend', id: 37} */ ] export function MultiSelectStory(): JSX.Element { From 2dbe48b79b0b761f6f24f0be916ec2eb48117a16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Thu, 23 Jun 2022 15:27:09 +0000 Subject: [PATCH 14/34] Update stories, Add ESC reset functionality --- src/SelectPanel/SelectPanel.tsx | 13 ++++++---- src/stories/SelectPanel.stories.tsx | 39 ++++++----------------------- 2 files changed, 16 insertions(+), 36 deletions(-) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index 43bcf01b939..cbba1326a89 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -172,10 +172,13 @@ export function SelectPanel({ onClose('selection') }, [finalItemsSelected, onSelectedChange, onClose, selected]) - const onCancelClickHandler = React.useCallback(() => { - setFinalItemsSelected(selectedItems) - onClose('escape') - }, [onClose, selectedItems]) + const onCancelClickHandler = React.useCallback( + (gesture?: 'anchor-click' | 'click-outside' | 'escape') => { + setFinalItemsSelected(selectedItems) + onClose(gesture ?? 'escape') + }, + [onClose, selectedItems] + ) const inputRef = React.useRef(null) const titleId = useSSRSafeId() @@ -209,7 +212,7 @@ export function SelectPanel({ anchorRef={anchorRef} open={open} onOpen={onOpen} - onClose={onClose} + onClose={onCancelClickHandler} overlayProps={{...overlayProps, onKeyPress: overlayKeyPressHandler, role: 'dialog', 'aria-labelledby': titleId}} focusTrapSettings={focusTrapSettings} focusZoneSettings={focusZoneSettings} diff --git a/src/stories/SelectPanel.stories.tsx b/src/stories/SelectPanel.stories.tsx index 2f85cae7f4a..eec6881a346 100644 --- a/src/stories/SelectPanel.stories.tsx +++ b/src/stories/SelectPanel.stories.tsx @@ -67,13 +67,10 @@ export function MultiSelectStory(): JSX.Element {

        Multi Select Panel

        Please select labels that describe your issue:
        ( - - {children ?? 'Select Labels'} - - )} + renderAnchor={({...anchorProps}) => Select Labels} title="Select Labels" inputLabel="Filter Labels" + placeholderText="design..." open={open} onOpenChange={setOpen} items={filteredItems} @@ -99,11 +96,7 @@ export function SingleSelectStory(): JSX.Element {

        Single Select Panel

        Please select a label that describe your issue:
        ( - - {children ?? 'Select Labels'} - - )} + renderAnchor={({...anchorProps}) => Select Label} title="Select a Label" inputLabel="Filter Labels" open={open} @@ -131,7 +124,7 @@ export function ExternalAnchorStory(): JSX.Element { <>

        Select Panel With External Anchor

        setOpen(!open)}> - Custom: {selected?.text || 'Click Me'} + Select a Label Single Select Panel
        Please select a label that describe your issue:
        ( - - {children ?? 'Select Labels'} - - )} + renderAnchor={({...anchorProps}) => Select Label} title="Select a Label" inputLabel="Filter Labels" open={open} @@ -196,11 +185,7 @@ export function SelectPanelWithUnderflowingItemsStory(): JSX.Element {

        Single Select Panel

        Please select a label that describe your issue:
        ( - - {children ?? 'Select Labels'} - - )} + renderAnchor={({...anchorProps}) => Select Label} title="Select a Label" inputLabel="Filter Labels" open={open} @@ -239,11 +224,7 @@ export function SelectPanelWithUnderflowingItemsAfterFetch(): JSX.Element {

        Single Select Panel

        Please select a label that describe your issue:
        ( - - {children ?? 'Select Labels'} - - )} + renderAnchor={({...anchorProps}) => Select Label} title="Select a Label" inputLabel="Filter Labels" open={open} @@ -272,11 +253,7 @@ export function SelectPanelAboveTallBody(): JSX.Element {

        Single Select Panel

        Please select a label that describe your issue:
        ( - - {children ?? 'Select Labels'} - - )} + renderAnchor={({...anchorProps}) => Select Label} title="Select a Label" inputLabel="Filter Labels" open={open} From d28b90dfcfca4734ed5a610348273195743c40c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Bolonio?= Date: Mon, 27 Jun 2022 09:30:27 +0000 Subject: [PATCH 15/34] Add changeset (breaking change) --- .changeset/grumpy-carrots-admire.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/grumpy-carrots-admire.md diff --git a/.changeset/grumpy-carrots-admire.md b/.changeset/grumpy-carrots-admire.md new file mode 100644 index 00000000000..330bade2dd2 --- /dev/null +++ b/.changeset/grumpy-carrots-admire.md @@ -0,0 +1,5 @@ +--- +'@primer/react': major +--- + +Accessibility fixes for SelectPanel. This is a **breaking change** for the SelectPanel because there are two new required props (`title` and `inputLabel`). From 4c54d8a40c265a508a0fd87bee285e9685c673d9 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Mon, 27 Jun 2022 15:36:26 +0000 Subject: [PATCH 16/34] Preselect items on the SelectPanel docs that aren't the first ones --- docs/content/SelectPanel.mdx | 2 +- src/stories/SelectPanel.stories.tsx | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/content/SelectPanel.mdx b/docs/content/SelectPanel.mdx index 94d51d9ba12..84a8c3f734a 100644 --- a/docs/content/SelectPanel.mdx +++ b/docs/content/SelectPanel.mdx @@ -37,7 +37,7 @@ const items = [ ] function DemoComponent() { - const [selected, setSelected] = React.useState([items[0], items[1]]) + const [selected, setSelected] = React.useState([items[2], items[4]]) const [filter, setFilter] = React.useState('') const filteredItems = items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) const [open, setOpen] = React.useState(false) diff --git a/src/stories/SelectPanel.stories.tsx b/src/stories/SelectPanel.stories.tsx index eec6881a346..180073c186e 100644 --- a/src/stories/SelectPanel.stories.tsx +++ b/src/stories/SelectPanel.stories.tsx @@ -57,7 +57,7 @@ const items = [ ] export function MultiSelectStory(): JSX.Element { - const [selected, setSelected] = React.useState([items[0], items[1]]) + const [selected, setSelected] = React.useState([items[2], items[4]]) const [filter, setFilter] = React.useState('') const filteredItems = items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) const [open, setOpen] = useState(false) @@ -86,7 +86,7 @@ export function MultiSelectStory(): JSX.Element { MultiSelectStory.storyName = 'Multi Select' export function SingleSelectStory(): JSX.Element { - const [selected, setSelected] = React.useState(items[0]) + const [selected, setSelected] = React.useState(items[2]) const [filter, setFilter] = React.useState('') const filteredItems = items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) const [open, setOpen] = useState(false) @@ -114,7 +114,7 @@ export function SingleSelectStory(): JSX.Element { SingleSelectStory.storyName = 'Single Select' export function ExternalAnchorStory(): JSX.Element { - const [selected, setSelected] = React.useState(items[0]) + const [selected, setSelected] = React.useState(items[1]) const [filter, setFilter] = React.useState('') const filteredItems = items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) const [open, setOpen] = useState(false) @@ -146,7 +146,7 @@ export function ExternalAnchorStory(): JSX.Element { ExternalAnchorStory.storyName = 'With External Anchor' export function SelectPanelWithOverflowingItemsStory(): JSX.Element { - const [selected, setSelected] = React.useState(items[0]) + const [selected, setSelected] = React.useState(items[3]) const [filter, setFilter] = React.useState('') const filteredItems = items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) const [open, setOpen] = useState(false) @@ -175,7 +175,7 @@ SelectPanelWithOverflowingItemsStory.storyName = 'SelectPanel, Overflowing Items export function SelectPanelWithUnderflowingItemsStory(): JSX.Element { const underflowingItems = [items[0], items[1]] - const [selected, setSelected] = React.useState(underflowingItems[0]) + const [selected, setSelected] = React.useState(underflowingItems[1]) const [filter, setFilter] = React.useState('') const filteredItems = underflowingItems.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) const [open, setOpen] = useState(false) @@ -203,7 +203,7 @@ export function SelectPanelWithUnderflowingItemsStory(): JSX.Element { SelectPanelWithUnderflowingItemsStory.storyName = 'SelectPanel, Underflowing Items' export function SelectPanelWithUnderflowingItemsAfterFetch(): JSX.Element { - const [selected, setSelected] = React.useState(items[0]) + const [selected, setSelected] = React.useState(items[1]) const [filter, setFilter] = React.useState('') const [fetchedItems, setFetchedItems] = useState([]) const filteredItems = React.useMemo( @@ -243,7 +243,7 @@ export function SelectPanelWithUnderflowingItemsAfterFetch(): JSX.Element { SelectPanelWithUnderflowingItemsAfterFetch.storyName = 'SelectPanel, Underflowing Items (After Fetch)' export function SelectPanelAboveTallBody(): JSX.Element { - const [selected, setSelected] = React.useState(items[0]) + const [selected, setSelected] = React.useState(items[1]) const [filter, setFilter] = React.useState('') const filteredItems = items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) const [open, setOpen] = useState(false) From 81bd88625f6001f498f0cda2196ac5f899560109 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Mon, 27 Jun 2022 15:51:06 +0000 Subject: [PATCH 17/34] Use Heading instead of h1 on SelectPanel --- src/SelectPanel/SelectPanel.tsx | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index cbba1326a89..f1dc33ba8ea 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -2,6 +2,7 @@ import React, {useCallback, useMemo, useState} from 'react' import {FilteredActionList, FilteredActionListProps} from '../FilteredActionList' import {OverlayProps} from '../Overlay' import {ItemInput} from '../deprecated/ActionList/List' +import Heading from '../Heading' import {FocusZoneHookSettings} from '../hooks/useFocusZone' import {DropdownButton} from '../deprecated/DropdownMenu' import {ItemProps} from '../deprecated/ActionList' @@ -67,13 +68,6 @@ const SrOnly = styled.span` border: 0; ` -const Title = styled.h1` - font-size: ${get('fontSizes.1')}; - font-weight: ${get('fontWeights.bold')}; - margin: 0; /* override default margin */ - ${sx}; -` - export function SelectPanel({ open, onOpenChange, @@ -227,7 +221,7 @@ export function SelectPanel({ : `${items.length} matching ${items.length === 1 ? 'item' : 'items'}`} - {title} + {title} Date: Mon, 27 Jun 2022 16:08:47 +0000 Subject: [PATCH 18/34] Don't use selected items as the SelectPanel anchor label --- docs/content/SelectPanel.mdx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/content/SelectPanel.mdx b/docs/content/SelectPanel.mdx index 84a8c3f734a..6e958296e01 100644 --- a/docs/content/SelectPanel.mdx +++ b/docs/content/SelectPanel.mdx @@ -44,9 +44,9 @@ function DemoComponent() { return ( ( - )} title="Select Items" From db16c680854edcccd1b26e948f375d01eb4c7461 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Mon, 27 Jun 2022 16:12:18 +0000 Subject: [PATCH 19/34] Replace custom SrOnly with VisuallyHidden --- src/FilteredActionList/FilteredActionList.tsx | 15 ++---------- src/SelectPanel/SelectPanel.tsx | 24 +++++-------------- 2 files changed, 8 insertions(+), 31 deletions(-) diff --git a/src/FilteredActionList/FilteredActionList.tsx b/src/FilteredActionList/FilteredActionList.tsx index 84dd513fdec..7ff89e56ead 100644 --- a/src/FilteredActionList/FilteredActionList.tsx +++ b/src/FilteredActionList/FilteredActionList.tsx @@ -13,6 +13,7 @@ import useScrollFlash from '../hooks/useScrollFlash' import {scrollIntoView} from '@primer/behaviors' import type {ScrollIntoViewOptions} from '@primer/behaviors' import {SxProp} from '../sx' +import VisuallyHidden from '../_VisuallyHidden' const menuScrollMargins: ScrollIntoViewOptions = {startMargin: 0, endMargin: 8} @@ -33,18 +34,6 @@ const StyledHeader = styled.div` z-index: 1; ` -// sr-only -const SrOnly = styled.span` - position: absolute; - width: 1px; - height: 1px; - padding: 0; - margin: -1px; - overflow: hidden; - clip: rect(0, 0, 0, 0); - border: 0; -` - export function FilteredActionList({ loading = false, placeholderText, @@ -111,7 +100,7 @@ export function FilteredActionList({ aria-describedby={inputDescriptionTextId} {...textInputProps} /> - Items will be filtered as you type + Items will be filtered as you type {loading ? ( diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index f1dc33ba8ea..f2d2ec2d2a9 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -11,13 +11,11 @@ import {TextInputProps} from '../TextInput' import {useProvidedStateOrCreate} from '../hooks/useProvidedStateOrCreate' import {AnchoredOverlayWrapperAnchorProps} from '../AnchoredOverlay/AnchoredOverlay' import {useProvidedRefOrCreate} from '../hooks' -import styled from 'styled-components' import {Button, IconButton} from '../Button' import {SearchIcon, XIcon} from '@primer/octicons-react' -import sx, {SxProp} from '../sx' -import {get} from '../constants' import Box from '../Box' import {useSSRSafeId} from '@react-aria/ssr' +import VisuallyHidden from '../_VisuallyHidden' interface SelectPanelSingleSelection { selected: ItemInput | undefined @@ -56,18 +54,6 @@ const focusZoneSettings: Partial = { disabled: true } -// sr-only -const SrOnly = styled.span` - position: absolute; - width: 1px; - height: 1px; - padding: 0; - margin: -1px; - overflow: hidden; - clip: rect(0, 0, 0, 0); - border: 0; -` - export function SelectPanel({ open, onOpenChange, @@ -213,15 +199,17 @@ export function SelectPanel({ > {/* inheriting height and maxHeight ensures that the content is never taller than the Overlay (which would break scrolling the items) */} - + {filterValue === '' ? 'Showing all items' : items.length <= 0 ? 'No matching items' : `${items.length} matching ${items.length === 1 ? 'item' : 'items'}`} - + - {title} + + {title} + Date: Mon, 27 Jun 2022 16:15:25 +0000 Subject: [PATCH 20/34] Make the SelectPanel example dimensions larger --- docs/content/SelectPanel.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/SelectPanel.mdx b/docs/content/SelectPanel.mdx index 6e958296e01..c2067da79cf 100644 --- a/docs/content/SelectPanel.mdx +++ b/docs/content/SelectPanel.mdx @@ -58,7 +58,7 @@ function DemoComponent() { onSelectedChange={setSelected} onFilterChange={setFilter} showItemDividers={true} - overlayProps={{width: 'small', height: 'xsmall'}} + overlayProps={{width: 'medium', height: 'medium'}} /> ) } From 7d01c0daa735402e8323b81f652e9d9a6d3258b3 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Mon, 27 Jun 2022 16:47:30 +0000 Subject: [PATCH 21/34] Support setting the selected items asyncronously --- src/SelectPanel/SelectPanel.tsx | 4 ++++ src/stories/SelectPanel.stories.tsx | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index f2d2ec2d2a9..fe98ee8c772 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -87,6 +87,10 @@ export function SelectPanel({ const [finalItemsSelected, setFinalItemsSelected] = useState(selectedItems) + // Refresh the selected items state when the prop changes. + // This is necessary because sometimes the selected items need to be fetched async. + React.useEffect(() => setFinalItemsSelected(selectedItems), [selectedItems]) + const anchorRef = useProvidedRefOrCreate(externalAnchorRef) const onOpen: AnchoredOverlayProps['onOpen'] = useCallback(gesture => onOpenChange(true, gesture), [onOpenChange]) const onClose = useCallback( diff --git a/src/stories/SelectPanel.stories.tsx b/src/stories/SelectPanel.stories.tsx index 180073c186e..e804cbaa186 100644 --- a/src/stories/SelectPanel.stories.tsx +++ b/src/stories/SelectPanel.stories.tsx @@ -203,7 +203,7 @@ export function SelectPanelWithUnderflowingItemsStory(): JSX.Element { SelectPanelWithUnderflowingItemsStory.storyName = 'SelectPanel, Underflowing Items' export function SelectPanelWithUnderflowingItemsAfterFetch(): JSX.Element { - const [selected, setSelected] = React.useState(items[1]) + const [selected, setSelected] = React.useState(undefined) const [filter, setFilter] = React.useState('') const [fetchedItems, setFetchedItems] = useState([]) const filteredItems = React.useMemo( @@ -216,6 +216,7 @@ export function SelectPanelWithUnderflowingItemsAfterFetch(): JSX.Element { setOpen(!open) setTimeout(() => { setFetchedItems([items[0], items[1]]) + setSelected(items[1]) // Sometimes the selected items need to be fetched async too. }, 1500) } From 0144a2116669e53e25aeed2729f1703db9f93834 Mon Sep 17 00:00:00 2001 From: Cameron McHenry Date: Tue, 28 Jun 2022 11:39:05 -0400 Subject: [PATCH 22/34] Fix new select panel issues (#2147) * Fix onClose being called with event instead of gesture Co-authored-by: Hector Garcia --- src/SelectPanel/SelectPanel.tsx | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index fe98ee8c772..e9888062f3f 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -156,7 +156,7 @@ export function SelectPanel({ onClose('selection') }, [finalItemsSelected, onSelectedChange, onClose, selected]) - const onCancelClickHandler = React.useCallback( + const onCloseOverlay = React.useCallback( (gesture?: 'anchor-click' | 'click-outside' | 'escape') => { setFinalItemsSelected(selectedItems) onClose(gesture ?? 'escape') @@ -164,6 +164,11 @@ export function SelectPanel({ [onClose, selectedItems] ) + const onCloseClickHandler = React.useCallback(() => { + setFinalItemsSelected(selectedItems) + onClose('escape') + }, [onClose, selectedItems]) + const inputRef = React.useRef(null) const titleId = useSSRSafeId() const focusTrapSettings = { @@ -196,7 +201,7 @@ export function SelectPanel({ anchorRef={anchorRef} open={open} onOpen={onOpen} - onClose={onCancelClickHandler} + onClose={onCloseOverlay} overlayProps={{...overlayProps, onKeyPress: overlayKeyPressHandler, role: 'dialog', 'aria-labelledby': titleId}} focusTrapSettings={focusTrapSettings} focusZoneSettings={focusZoneSettings} @@ -214,7 +219,7 @@ export function SelectPanel({ {title} - + - + From f6f20435c4155c1b91aa15cfd9c10c66fec83b2f Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Fri, 1 Jul 2022 11:08:28 +0000 Subject: [PATCH 23/34] Remove unsupported PageUpDown bindkeys from useFocusZone --- src/deprecated/ActionList/List.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deprecated/ActionList/List.tsx b/src/deprecated/ActionList/List.tsx index 6afd838d3b9..3cf21f3abd0 100644 --- a/src/deprecated/ActionList/List.tsx +++ b/src/deprecated/ActionList/List.tsx @@ -230,7 +230,7 @@ export const List = React.forwardRef((props, forwar groups = [...groupMap.values()] } - const {containerRef} = useFocusZone({bindKeys: FocusKeys.ArrowVertical | FocusKeys.HomeAndEnd | FocusKeys.PageUpDown}) + const {containerRef} = useFocusZone({bindKeys: FocusKeys.ArrowVertical | FocusKeys.HomeAndEnd}) return (
        }> From a304177d8fb0a1e0fe12eba5607a0d65187d97fa Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Fri, 1 Jul 2022 15:52:19 +0200 Subject: [PATCH 24/34] Remove console.debug --- src/SelectPanel/SelectPanel.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index e9888062f3f..03bd9410b67 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -142,9 +142,6 @@ export function SelectPanel({ }) }, [items, selected, setFinalItemsSelected, finalItemsSelected]) - // eslint-disable-next-line no-console - React.useEffect(() => console.debug({finalItemsSelected}), [finalItemsSelected]) - const onSaveClickHandler = React.useCallback(() => { if (isMultiSelectVariant(selected)) { const multiSelectOnChange = onSelectedChange as SelectPanelMultiSelection['onSelectedChange'] From e3f07a6f3a489a985c58af6b38c0f33d8906783f Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Thu, 18 Aug 2022 16:36:48 +0200 Subject: [PATCH 25/34] Make `title` and `inputLabel` optional with defaults --- .changeset/grumpy-carrots-admire.md | 4 ++-- src/SelectPanel/SelectPanel.tsx | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/.changeset/grumpy-carrots-admire.md b/.changeset/grumpy-carrots-admire.md index 330bade2dd2..0e8708d6a16 100644 --- a/.changeset/grumpy-carrots-admire.md +++ b/.changeset/grumpy-carrots-admire.md @@ -1,5 +1,5 @@ --- -'@primer/react': major +'@primer/react': minor --- -Accessibility fixes for SelectPanel. This is a **breaking change** for the SelectPanel because there are two new required props (`title` and `inputLabel`). +Accessibility fixes for SelectPanel. diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index 03bd9410b67..1d903c4d147 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -32,8 +32,10 @@ interface SelectPanelBaseProps { open: boolean, gesture: 'anchor-click' | 'anchor-key-press' | 'click-outside' | 'escape' | 'selection' ) => void - title: string - inputLabel: string + // TODO: Make `title` and `inputLabel` required and remove default values + // in the next major release + title?: string + inputLabel?: string overlayProps?: Partial } @@ -59,8 +61,8 @@ export function SelectPanel({ onOpenChange, renderAnchor = props => , anchorRef: externalAnchorRef, - title, - inputLabel, + title = 'Select an item', + inputLabel = 'Filter items', selected, onSelectedChange, filterValue: externalFilterValue, From ce4715ddad3572158e565ef504e9ef2f609577b0 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Thu, 18 Aug 2022 16:37:03 +0200 Subject: [PATCH 26/34] Remove unused prop --- src/stories/SelectPanel.stories.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/stories/SelectPanel.stories.tsx b/src/stories/SelectPanel.stories.tsx index 17d0893e6a3..c3653dcf80b 100644 --- a/src/stories/SelectPanel.stories.tsx +++ b/src/stories/SelectPanel.stories.tsx @@ -70,7 +70,6 @@ export function MultiSelectStory(): JSX.Element { renderAnchor={({...anchorProps}) => Select Labels} title="Select Labels" inputLabel="Filter Labels" - placeholderText="design..." open={open} onOpenChange={setOpen} items={filteredItems} From 2669c4fd4e0fc54708f8d24368f50e09a8e8bde4 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Thu, 18 Aug 2022 16:38:07 +0200 Subject: [PATCH 27/34] Add a placeholder prop for the search input --- src/SelectPanel/SelectPanel.tsx | 3 +++ src/stories/SelectPanel.stories.tsx | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index 1d903c4d147..cf6dbd36b6b 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -36,6 +36,7 @@ interface SelectPanelBaseProps { // in the next major release title?: string inputLabel?: string + inputPlaceholder?: string overlayProps?: Partial } @@ -63,6 +64,7 @@ export function SelectPanel({ anchorRef: externalAnchorRef, title = 'Select an item', inputLabel = 'Filter items', + inputPlaceholder, selected, onSelectedChange, filterValue: externalFilterValue, @@ -180,6 +182,7 @@ export function SelectPanel({ contrast: true, leadingVisual: SearchIcon, 'aria-label': inputLabel, + placeholder: inputPlaceholder, ...textInputProps } }, [textInputProps, inputLabel]) diff --git a/src/stories/SelectPanel.stories.tsx b/src/stories/SelectPanel.stories.tsx index c3653dcf80b..2ad9f6179fe 100644 --- a/src/stories/SelectPanel.stories.tsx +++ b/src/stories/SelectPanel.stories.tsx @@ -70,6 +70,7 @@ export function MultiSelectStory(): JSX.Element { renderAnchor={({...anchorProps}) => Select Labels} title="Select Labels" inputLabel="Filter Labels" + inputPlaceholder="design..." open={open} onOpenChange={setOpen} items={filteredItems} @@ -98,6 +99,7 @@ export function SingleSelectStory(): JSX.Element { renderAnchor={({...anchorProps}) => Select Label} title="Select a Label" inputLabel="Filter Labels" + inputPlaceholder="design..." open={open} onOpenChange={setOpen} items={filteredItems} @@ -130,6 +132,7 @@ export function ExternalAnchorStory(): JSX.Element { anchorRef={buttonRef} title="Select a Label" inputLabel="Filter Labels" + inputPlaceholder="design..." open={open} onOpenChange={setOpen} items={filteredItems} @@ -158,6 +161,7 @@ export function SelectPanelWithOverflowingItemsStory(): JSX.Element { renderAnchor={({...anchorProps}) => Select Label} title="Select a Label" inputLabel="Filter Labels" + inputPlaceholder="design..." open={open} onOpenChange={setOpen} items={filteredItems} @@ -187,6 +191,7 @@ export function SelectPanelWithUnderflowingItemsStory(): JSX.Element { renderAnchor={({...anchorProps}) => Select Label} title="Select a Label" inputLabel="Filter Labels" + inputPlaceholder="design..." open={open} onOpenChange={setOpen} items={filteredItems} @@ -227,6 +232,7 @@ export function SelectPanelWithUnderflowingItemsAfterFetch(): JSX.Element { renderAnchor={({...anchorProps}) => Select Label} title="Select a Label" inputLabel="Filter Labels" + inputPlaceholder="design..." open={open} onOpenChange={onOpenChange} loading={filteredItems.length === 0} @@ -256,6 +262,7 @@ export function SelectPanelAboveTallBody(): JSX.Element { renderAnchor={({...anchorProps}) => Select Label} title="Select a Label" inputLabel="Filter Labels" + inputPlaceholder="design..." open={open} onOpenChange={setOpen} items={filteredItems} From a838425984bd8876c10fa523b0814e96673605d6 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Thu, 18 Aug 2022 16:48:43 +0200 Subject: [PATCH 28/34] Make footer buttons smaller --- src/SelectPanel/SelectPanel.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index cf6dbd36b6b..8827d864229 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -241,13 +241,15 @@ export function SelectPanel({ alignItems="center" justifyContent="flex-end" gridGap="8px" - padding="16px" + padding="12px" borderTopColor="border.default" borderTopWidth={1} borderTopStyle="solid" > - - + From a4d209e2177f646235e7f340f389168c5be68745 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Thu, 18 Aug 2022 17:02:24 +0200 Subject: [PATCH 29/34] Use `ButtonClose` --- src/SelectPanel/SelectPanel.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index 8827d864229..f22565853e0 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -11,11 +11,12 @@ import {TextInputProps} from '../TextInput' import {useProvidedStateOrCreate} from '../hooks/useProvidedStateOrCreate' import {AnchoredOverlayWrapperAnchorProps} from '../AnchoredOverlay/AnchoredOverlay' import {useProvidedRefOrCreate} from '../hooks' -import {Button, IconButton} from '../Button' +import {Button} from '../Button' import {SearchIcon, XIcon} from '@primer/octicons-react' import Box from '../Box' import {useSSRSafeId} from '@react-aria/ssr' import VisuallyHidden from '../_VisuallyHidden' +import ButtonClose from '../deprecated/Button/ButtonClose' interface SelectPanelSingleSelection { selected: ItemInput | undefined @@ -221,7 +222,7 @@ export function SelectPanel({ {title} - + Date: Fri, 19 Aug 2022 17:54:37 +0200 Subject: [PATCH 30/34] Better default title for multiselect `SelectPanel` --- src/SelectPanel/SelectPanel.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index f22565853e0..85b0fef8d6f 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -63,11 +63,11 @@ export function SelectPanel({ onOpenChange, renderAnchor = props => , anchorRef: externalAnchorRef, - title = 'Select an item', - inputLabel = 'Filter items', - inputPlaceholder, selected, onSelectedChange, + title = isMultiSelectVariant(selected) ? 'Select items' : 'Select an item', + inputLabel = 'Filter items', + inputPlaceholder, filterValue: externalFilterValue, onFilterChange: externalOnFilterChange, items, From 266eab47277883c578167ce3c587c2dfce5639d9 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Tue, 23 Aug 2022 11:52:33 +0200 Subject: [PATCH 31/34] Fix lint warnings --- src/SelectPanel/SelectPanel.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/SelectPanel/SelectPanel.tsx b/src/SelectPanel/SelectPanel.tsx index 85b0fef8d6f..1d39cd2765e 100644 --- a/src/SelectPanel/SelectPanel.tsx +++ b/src/SelectPanel/SelectPanel.tsx @@ -12,7 +12,7 @@ import {useProvidedStateOrCreate} from '../hooks/useProvidedStateOrCreate' import {AnchoredOverlayWrapperAnchorProps} from '../AnchoredOverlay/AnchoredOverlay' import {useProvidedRefOrCreate} from '../hooks' import {Button} from '../Button' -import {SearchIcon, XIcon} from '@primer/octicons-react' +import {SearchIcon} from '@primer/octicons-react' import Box from '../Box' import {useSSRSafeId} from '@react-aria/ssr' import VisuallyHidden from '../_VisuallyHidden' @@ -186,7 +186,7 @@ export function SelectPanel({ placeholder: inputPlaceholder, ...textInputProps } - }, [textInputProps, inputLabel]) + }, [textInputProps, inputLabel, inputPlaceholder]) const overlayKeyPressHandler = useCallback( event => { From e6f6297d7b3f8227e64267387362852ac28b9f12 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Thu, 25 Aug 2022 20:17:16 +0200 Subject: [PATCH 32/34] Fix the AutocompleteMenu tests --- src/Autocomplete/AutocompleteMenu.tsx | 2 ++ src/deprecated/ActionList/Item.tsx | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/Autocomplete/AutocompleteMenu.tsx b/src/Autocomplete/AutocompleteMenu.tsx index 2171279b64e..f21a5441bac 100644 --- a/src/Autocomplete/AutocompleteMenu.tsx +++ b/src/Autocomplete/AutocompleteMenu.tsx @@ -151,6 +151,7 @@ function AutocompleteMenu(props: AutocompleteMe items.map(selectableItem => { return { ...selectableItem, + _legacyEnterSupport: true, //TODO: Change behaviour, the enter key should not be used here. role: 'option', id: selectableItem.id, selected: selectionVariant === 'multiple' ? selectedItemIds.includes(selectableItem.id) : undefined, @@ -217,6 +218,7 @@ function AutocompleteMenu(props: AutocompleteMe ? [ { ...addNewItem, + _legacyEnterSupport: true, //TODO: Change behaviour, the enter key should not be used here. leadingVisual: () => , onAction: (item: T) => { // TODO: make it possible to pass a leadingVisual when using `addNewItem` diff --git a/src/deprecated/ActionList/Item.tsx b/src/deprecated/ActionList/Item.tsx index 59fbaaa03ad..cd3da4daceb 100644 --- a/src/deprecated/ActionList/Item.tsx +++ b/src/deprecated/ActionList/Item.tsx @@ -118,6 +118,11 @@ export interface ItemProps extends SxProp { * An item to pass back in the `onAction` callback, meant as */ item?: ItemInput + + /** + * @deprecated Allows the `enter` key to select an item. Kept for compatibility purposes. Should not be used in new components. + */ + _legacyEnterSupport?: boolean } const getItemVariant = (variant = 'default', disabled?: boolean) => { @@ -349,6 +354,7 @@ export const Item = React.forwardRef((itemProps, ref) => { children, onClick, id, + _legacyEnterSupport = false, ...props } = itemProps @@ -362,12 +368,16 @@ export const Item = React.forwardRef((itemProps, ref) => { } onKeyPress?.(event) - if (!event.defaultPrevented && [' '].includes(event.key)) { + const triggerKeys = [' '] + if (_legacyEnterSupport == true) { + triggerKeys.push('Enter') + } + if (!event.defaultPrevented && triggerKeys.includes(event.key)) { onAction?.(itemProps, event) event.preventDefault() } }, - [onAction, disabled, itemProps, onKeyPress] + [onAction, disabled, itemProps, onKeyPress, _legacyEnterSupport] ) const clickHandler = useCallback( From 9c7e894b4e4fcd056bf2b0c54787f11394807ed9 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Thu, 25 Aug 2022 20:22:03 +0200 Subject: [PATCH 33/34] Fix lint error --- src/deprecated/ActionList/Item.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deprecated/ActionList/Item.tsx b/src/deprecated/ActionList/Item.tsx index cd3da4daceb..64adbc2be76 100644 --- a/src/deprecated/ActionList/Item.tsx +++ b/src/deprecated/ActionList/Item.tsx @@ -369,7 +369,7 @@ export const Item = React.forwardRef((itemProps, ref) => { onKeyPress?.(event) const triggerKeys = [' '] - if (_legacyEnterSupport == true) { + if (_legacyEnterSupport === true) { triggerKeys.push('Enter') } if (!event.defaultPrevented && triggerKeys.includes(event.key)) { From 54db5ef43d52565c68d18ac2fa469dd23fc0121b Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Thu, 25 Aug 2022 21:40:16 +0200 Subject: [PATCH 34/34] Fix MarkdownEditor tests, remove one for old behavior --- src/drafts/MarkdownEditor/MarkdownEditor.test.tsx | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx b/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx index a3f3d7ad16a..0201f8e3eba 100644 --- a/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx +++ b/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx @@ -1081,13 +1081,6 @@ describe('MarkdownEditor', () => { expect(queryByRole('listbox')).not.toBeInTheDocument() }) - it('autofocuses filter and filters replies based only on name', async () => { - const {getToolbarButton, getByRole, user} = await render() - await user.click(getToolbarButton(buttonLabel)) - await user.keyboard('Thanks') - expect(within(getByRole('listbox')).getAllByRole('option')).toHaveLength(1) - }) - it('inserts the selected reply at the caret position, closes the menu, and focuses the input', async () => { const {getToolbarButton, getInput, user, queryByRole} = await render( @@ -1098,7 +1091,7 @@ describe('MarkdownEditor', () => { input.setSelectionRange(10, 10) await user.click(getToolbarButton(buttonLabel)) - await user.keyboard('Thanks{Enter}') + await user.keyboard('{Control>}3{/Control}') expect(queryByRole('listbox')).not.toBeInTheDocument() await waitFor(() => expect(getInput().value).toBe('preceding Thanks for your contribution! following'))