From b976cb31d7536c02f183e12c0283d15fd948e6a7 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Thu, 7 Sep 2023 14:41:52 +0100 Subject: [PATCH 01/25] Refactoring `CircularOptionPicker` Added some tests and a new story to go with the changes. --- .../src/circular-option-picker/index.tsx | 262 +----------------- .../option-picker-actions.tsx | 60 ++++ .../option-picker-context.tsx | 6 + .../option-picker-option-group.tsx | 33 +++ .../option-picker-option.tsx | 118 ++++++++ .../circular-option-picker/option-picker.tsx | 167 +++++++++++ .../stories/index.story.tsx | 21 +- .../src/circular-option-picker/test/index.tsx | 133 +++++++++ .../src/circular-option-picker/types.ts | 47 +++- 9 files changed, 567 insertions(+), 280 deletions(-) create mode 100644 packages/components/src/circular-option-picker/option-picker-actions.tsx create mode 100644 packages/components/src/circular-option-picker/option-picker-context.tsx create mode 100644 packages/components/src/circular-option-picker/option-picker-option-group.tsx create mode 100644 packages/components/src/circular-option-picker/option-picker-option.tsx create mode 100644 packages/components/src/circular-option-picker/option-picker.tsx create mode 100644 packages/components/src/circular-option-picker/test/index.tsx diff --git a/packages/components/src/circular-option-picker/index.tsx b/packages/components/src/circular-option-picker/index.tsx index e32cfb822808c8..d58ee44430dd4d 100644 --- a/packages/components/src/circular-option-picker/index.tsx +++ b/packages/components/src/circular-option-picker/index.tsx @@ -1,264 +1,10 @@ -/** - * External dependencies - */ -import classnames from 'classnames'; - -/** - * WordPress dependencies - */ -import { useInstanceId } from '@wordpress/compose'; -import { createContext, useContext, useEffect } from '@wordpress/element'; -import { Icon, check } from '@wordpress/icons'; -import { isRTL } from '@wordpress/i18n'; - /** * Internal dependencies */ -import Button from '../button'; -import { Composite, CompositeItem, useCompositeState } from '../composite'; -import Dropdown from '../dropdown'; -import Tooltip from '../tooltip'; -import type { - CircularOptionPickerProps, - DropdownLinkActionProps, - OptionGroupProps, - OptionProps, -} from './types'; -import type { WordPressComponentProps } from '../ui/context'; -import type { ButtonAsButtonProps } from '../button/types'; - -const CircularOptionPickerContext = createContext( {} ); - -const hasSelectedOption = new Map(); - -export function Option( { - className, - isSelected, - selectedIconProps, - tooltipText, - ...additionalProps -}: OptionProps ) { - const compositeState = useContext( CircularOptionPickerContext ); - const { - baseId = 'option', - currentId, - setCurrentId, - } = compositeState as any; - const id = useInstanceId( Option, baseId ); - - useEffect( () => { - // If we call `setCurrentId` here, it doesn't update for other - // Option renders in the same pass. So we have to store our own - // map to make sure that we only set the first selected option. - // We still need to check `currentId` because the control will - // update this as the user moves around, and that state should - // be maintained as the group gains and loses focus. - if ( isSelected && ! currentId && ! hasSelectedOption.get( baseId ) ) { - hasSelectedOption.set( baseId, true ); - setCurrentId( id ); - } - } ); - - const optionControl = ( - - ); - - return ( -
- { tooltipText ? ( - { optionControl } - ) : ( - optionControl - ) } - { isSelected && ( - - ) } -
- ); -} - -export function OptionGroup( { - className, - options, - ...additionalProps -}: OptionGroupProps ) { - const { 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledby } = - additionalProps as any; - const role = ariaLabel || ariaLabelledby ? 'group' : undefined; - - return ( -
- { options } -
- ); -} - -export function DropdownLinkAction( { - buttonProps, - className, - dropdownProps, - linkText, -}: DropdownLinkActionProps ) { - return ( - ( - - ) } - { ...dropdownProps } - /> - ); -} - -export function ButtonAction( { - className, - children, - ...additionalProps -}: WordPressComponentProps< ButtonAsButtonProps, 'button', false > ) { - return ( - - ); -} - -/** - *`CircularOptionPicker` is a component that displays a set of options as circular buttons. - * - * ```jsx - * import { CircularOptionPicker } from '../circular-option-picker'; - * import { useState } from '@wordpress/element'; - * - * const Example = () => { - * const [ currentColor, setCurrentColor ] = useState(); - * const colors = [ - * { color: '#f00', name: 'Red' }, - * { color: '#0f0', name: 'Green' }, - * { color: '#00f', name: 'Blue' }, - * ]; - * const colorOptions = ( - * <> - * { colors.map( ( { color, name }, index ) => { - * return ( - * setCurrentColor( index ) } - * aria-label={ name } - * /> - * ); - * } ) } - * - * ); - * return ( - * setCurrentColor( undefined ) } - * > - * { 'Clear' } - * - * } - * /> - * ); - * }; - * ``` - */ - -function CircularOptionPicker( props: CircularOptionPickerProps ) { - const { - actions, - className, - id: idProp, - options, - children, - loop = true, - ...additionalProps - } = props; - const id = useInstanceId( CircularOptionPicker, 'option-picker', idProp ); - const rtl = isRTL(); - const compositeState = useCompositeState( { baseId: id, loop, rtl } ); - - return ( - - -
- { options } -
- { children } - { actions && ( -
- { actions } -
- ) } - - - ); -} +import CircularOptionPicker from './option-picker'; -CircularOptionPicker.Option = Option; -CircularOptionPicker.OptionGroup = OptionGroup; -CircularOptionPicker.ButtonAction = ButtonAction; -CircularOptionPicker.DropdownLinkAction = DropdownLinkAction; +export { Option } from './option-picker-option'; +export { OptionGroup } from './option-picker-option-group'; +export { ButtonAction, DropdownLinkAction } from './option-picker-actions'; export default CircularOptionPicker; diff --git a/packages/components/src/circular-option-picker/option-picker-actions.tsx b/packages/components/src/circular-option-picker/option-picker-actions.tsx new file mode 100644 index 00000000000000..2817569b0564ed --- /dev/null +++ b/packages/components/src/circular-option-picker/option-picker-actions.tsx @@ -0,0 +1,60 @@ +/** + * External dependencies + */ +import classnames from 'classnames'; + +/** + * Internal dependencies + */ +import Button from '../button'; +import Dropdown from '../dropdown'; +import type { DropdownLinkActionProps } from './types'; +import type { WordPressComponentProps } from '../ui/context'; +import type { ButtonAsButtonProps } from '../button/types'; + +export function DropdownLinkAction( { + buttonProps, + className, + dropdownProps, + linkText, +}: DropdownLinkActionProps ) { + return ( + ( + + ) } + { ...dropdownProps } + /> + ); +} + +export function ButtonAction( { + className, + children, + ...additionalProps +}: WordPressComponentProps< ButtonAsButtonProps, 'button', false > ) { + return ( + + ); +} diff --git a/packages/components/src/circular-option-picker/option-picker-context.tsx b/packages/components/src/circular-option-picker/option-picker-context.tsx new file mode 100644 index 00000000000000..b66a386471863e --- /dev/null +++ b/packages/components/src/circular-option-picker/option-picker-context.tsx @@ -0,0 +1,6 @@ +/** + * WordPress dependencies + */ +import { createContext } from '@wordpress/element'; + +export const CircularOptionPickerContext = createContext( {} ); diff --git a/packages/components/src/circular-option-picker/option-picker-option-group.tsx b/packages/components/src/circular-option-picker/option-picker-option-group.tsx new file mode 100644 index 00000000000000..b7ac44c1c01991 --- /dev/null +++ b/packages/components/src/circular-option-picker/option-picker-option-group.tsx @@ -0,0 +1,33 @@ +/** + * External dependencies + */ +import classnames from 'classnames'; + +/** + * Internal dependencies + */ +import type { OptionGroupProps } from './types'; + +export function OptionGroup( { + className, + options, + ...additionalProps +}: OptionGroupProps ) { + const { 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledby } = + additionalProps as any; + const role = ariaLabel || ariaLabelledby ? 'group' : undefined; + + return ( +
+ { options } +
+ ); +} diff --git a/packages/components/src/circular-option-picker/option-picker-option.tsx b/packages/components/src/circular-option-picker/option-picker-option.tsx new file mode 100644 index 00000000000000..863f24a1726c86 --- /dev/null +++ b/packages/components/src/circular-option-picker/option-picker-option.tsx @@ -0,0 +1,118 @@ +/** + * External dependencies + */ +import classnames from 'classnames'; + +/** + * WordPress dependencies + */ +import { useInstanceId } from '@wordpress/compose'; +import { useContext, useEffect } from '@wordpress/element'; +import { Icon, check } from '@wordpress/icons'; + +/** + * Internal dependencies + */ +import { CircularOptionPickerContext } from './option-picker-context'; +import Button from '../button'; +import { CompositeItem } from '../composite'; +import Tooltip from '../tooltip'; +import type { OptionProps } from './types'; + +const hasSelectedOption = new Map(); + +function renderOptionAsButton( props: { + className?: string; + isPressed?: boolean; +} ) { + return ; +} + +function renderOptionAsOption( props: { + id: string; + className?: string; + isSelected?: boolean; + compositeState: any; +} ) { + const { className, isSelected, compositeState, ...additionalProps } = props; + + return ( + + ); +} + +export function Option( { + className, + isSelected, + selectedIconProps, + tooltipText, + ...additionalProps +}: OptionProps ) { + const compositeState = useContext( CircularOptionPickerContext ); + const { baseId, currentId, setCurrentId } = compositeState as any; + const isComposite = !! baseId; + const id = useInstanceId( Option, baseId ); + + useEffect( () => { + // If we call `setCurrentId` here, it doesn't update for other + // Option renders in the same pass. So we have to store our own + // map to make sure that we only set the first selected option. + // We still need to check `currentId` because the control will + // update this as the user moves around, and that state should + // be maintained as the group gains and loses focus. + if ( + isComposite && + isSelected && + ! currentId && + ! hasSelectedOption.get( baseId ) + ) { + hasSelectedOption.set( baseId, true ); + setCurrentId( id ); + } + }, [ baseId, currentId, id, isComposite, isSelected, setCurrentId ] ); + + const optionControl = isComposite + ? renderOptionAsOption( { + id, + className: 'components-circular-option-picker__option', + isSelected, + compositeState, + ...additionalProps, + } ) + : renderOptionAsButton( { + id, + className: 'components-circular-option-picker__option', + isPressed: isSelected, + ...additionalProps, + } ); + + return ( +
+ { tooltipText ? ( + { optionControl } + ) : ( + optionControl + ) } + { isSelected && ( + + ) } +
+ ); +} diff --git a/packages/components/src/circular-option-picker/option-picker.tsx b/packages/components/src/circular-option-picker/option-picker.tsx new file mode 100644 index 00000000000000..25f7899f79f223 --- /dev/null +++ b/packages/components/src/circular-option-picker/option-picker.tsx @@ -0,0 +1,167 @@ +/** + * External dependencies + */ +import classnames from 'classnames'; + +/** + * WordPress dependencies + */ +import { useInstanceId } from '@wordpress/compose'; +import { useEffect } from '@wordpress/element'; +import { isRTL } from '@wordpress/i18n'; + +/** + * Internal dependencies + */ +import { CircularOptionPickerContext } from './option-picker-context'; +import { Composite, useCompositeState } from '../composite'; +import type { + CircularOptionPickerProps, + ListboxCircularOptionPickerProps, + ButtonsCircularOptionPickerProps, +} from './types'; +import { Option } from './option-picker-option'; +import { OptionGroup } from './option-picker-option-group'; +import { ButtonAction, DropdownLinkAction } from './option-picker-actions'; + +/** + *`CircularOptionPicker` is a component that displays a set of options as circular buttons. + * + * ```jsx + * import { CircularOptionPicker } from '../circular-option-picker'; + * import { useState } from '@wordpress/element'; + * + * const Example = () => { + * const [ currentColor, setCurrentColor ] = useState(); + * const colors = [ + * { color: '#f00', name: 'Red' }, + * { color: '#0f0', name: 'Green' }, + * { color: '#00f', name: 'Blue' }, + * ]; + * const colorOptions = ( + * <> + * { colors.map( ( { color, name }, index ) => { + * return ( + * setCurrentColor( index ) } + * aria-label={ name } + * /> + * ); + * } ) } + * + * ); + * return ( + * setCurrentColor( undefined ) } + * > + * { 'Clear' } + * + * } + * /> + * ); + * }; + * ``` + */ + +function ListboxCircularOptionPicker( + props: Omit< + ListboxCircularOptionPickerProps, + 'asButtons' | 'actions' | 'options' + > +) { + const { id, disableLooping, children, ...additionalProps } = props; + const baseId = useInstanceId( CircularOptionPicker, 'option-picker', id ); + const rtl = isRTL(); + const loop = ! disableLooping; + + const compositeState = useCompositeState( { baseId, loop, rtl } ); + + // This is necessary as `useCompositeState` is sealed after + // the first render, so although unlikely to happen, if a state + // property should change, we need to process it accordingly. + useEffect( () => { + compositeState.setBaseId( baseId ); + compositeState.setLoop( !! loop ); + compositeState.setRTL( rtl ); + }, [ compositeState, baseId, loop, rtl ] ); + + return ( + + + { children } + + + ); +} + +function ButtonsCircularOptionPicker( + props: Omit< + ButtonsCircularOptionPickerProps, + 'asButtons' | 'actions' | 'options' + > +) { + const { children, ...additionalProps } = props; + + // We're including an empty context here, so as to prevent + // any nesting issues. + return ( +
+ + { children } + +
+ ); +} + +function CircularOptionPicker( props: CircularOptionPickerProps ) { + const { + asButtons, + actions, + options, + children, + className, + ...additionalProps + } = props; + + const OptionPickerImplementation = asButtons + ? ButtonsCircularOptionPicker + : ListboxCircularOptionPicker; + + return ( + +
+ { options } +
+ { children } + { actions && ( +
+ { actions } +
+ ) } + + ); +} + +CircularOptionPicker.Option = Option; +CircularOptionPicker.OptionGroup = OptionGroup; +CircularOptionPicker.ButtonAction = ButtonAction; +CircularOptionPicker.DropdownLinkAction = DropdownLinkAction; + +export default CircularOptionPicker; diff --git a/packages/components/src/circular-option-picker/stories/index.story.tsx b/packages/components/src/circular-option-picker/stories/index.story.tsx index 4ad346b93cf5cc..c05a284340e997 100644 --- a/packages/components/src/circular-option-picker/stories/index.story.tsx +++ b/packages/components/src/circular-option-picker/stories/index.story.tsx @@ -9,11 +9,7 @@ import { useState, createContext, useContext } from '@wordpress/element'; /** * Internal dependencies */ -import { - ButtonAction, - default as CircularOptionPicker, - DropdownLinkAction, -} from '..'; +import CircularOptionPicker from '..'; const CircularOptionPickerStoryContext = createContext< { currentColor?: string; @@ -25,11 +21,14 @@ const meta: Meta< typeof CircularOptionPicker > = { component: CircularOptionPicker, subcomponents: { // @ts-expect-error - See https://github.com/storybookjs/storybook/issues/23170 - 'CircularOptionPicker.Option': Option, + 'CircularOptionPicker.Option': CircularOptionPicker.Option, // @ts-expect-error - See https://github.com/storybookjs/storybook/issues/23170 - 'CircularOptionPicker.ButtonAction': ButtonAction, + 'CircularOptionPicker.OptionGroup': CircularOptionPicker.OptionGroup, // @ts-expect-error - See https://github.com/storybookjs/storybook/issues/23170 - 'CircularOptionPicker.DropdownLinkAction': DropdownLinkAction, + 'CircularOptionPicker.ButtonAction': CircularOptionPicker.ButtonAction, + // @ts-expect-error - See https://github.com/storybookjs/storybook/issues/23170 + 'CircularOptionPicker.DropdownLinkAction': + CircularOptionPicker.DropdownLinkAction, }, argTypes: { actions: { control: { type: null } }, @@ -114,6 +113,12 @@ const Template: StoryFn< typeof CircularOptionPicker > = ( props ) => ( export const Default = Template.bind( {} ); Default.args = { options: }; +export const AsButtons = Template.bind( {} ); +AsButtons.args = { + ...Default.args, + asButtons: true, +}; + export const WithButtonAction = Template.bind( {} ); WithButtonAction.args = { ...Default.args, diff --git a/packages/components/src/circular-option-picker/test/index.tsx b/packages/components/src/circular-option-picker/test/index.tsx new file mode 100644 index 00000000000000..7ba03183d835fe --- /dev/null +++ b/packages/components/src/circular-option-picker/test/index.tsx @@ -0,0 +1,133 @@ +/** + * External dependencies + */ +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +/** + * Internal dependencies + */ +import CircularOptionPicker from '..'; + +const SINGLE_OPTION = [ ]; + +const MULTIPLE_OPTIONS = [ + , + , +]; + +const DEFAULT_PROPS = { + 'aria-label': 'Circular Option Picker', + options: SINGLE_OPTION, +}; + +function getOption( name: string ) { + return screen.getByRole( 'option', { name } ); +} + +describe( 'CircularOptionPicker', () => { + describe( 'when `asButtons` is not set', () => { + it( 'should render as a listbox', async () => { + render( ); + + expect( screen.getByRole( 'listbox' ) ).toBeInTheDocument(); + expect( screen.getByRole( 'option' ) ).toBeInTheDocument(); + expect( screen.queryByRole( 'button' ) ).not.toBeInTheDocument(); + } ); + } ); + + describe( 'when `asButtons` is false', () => { + it( 'should render as a listbox', async () => { + render( + + ); + + expect( screen.getByRole( 'listbox' ) ).toBeInTheDocument(); + expect( screen.getByRole( 'option' ) ).toBeInTheDocument(); + expect( screen.queryByRole( 'button' ) ).not.toBeInTheDocument(); + } ); + } ); + + describe( 'when `asButtons` is true', () => { + it( 'should render as buttons', async () => { + render( + + ); + + expect( screen.queryByRole( 'listbox' ) ).not.toBeInTheDocument(); + expect( screen.queryByRole( 'option' ) ).not.toBeInTheDocument(); + expect( screen.getByRole( 'button' ) ).toBeInTheDocument(); + } ); + } ); + + describe( 'when `disableLooping` is not set', () => { + it( 'should loop', async () => { + const user = userEvent.setup(); + + render( + + ); + + await user.tab(); + expect( getOption( 'Option One' ) ).toHaveFocus(); + await user.keyboard( '[ArrowRight]' ); + expect( getOption( 'Option Two' ) ).toHaveFocus(); + await user.keyboard( '[ArrowRight]' ); + expect( getOption( 'Option One' ) ).toHaveFocus(); + } ); + } ); + + describe( 'when `disableLooping` is false', () => { + it( 'should loop', async () => { + const user = userEvent.setup(); + + render( + + ); + + await user.tab(); + expect( getOption( 'Option One' ) ).toHaveFocus(); + await user.keyboard( '[ArrowRight]' ); + expect( getOption( 'Option Two' ) ).toHaveFocus(); + await user.keyboard( '[ArrowRight]' ); + expect( getOption( 'Option One' ) ).toHaveFocus(); + } ); + } ); + + describe( 'when `disableLooping` is true', () => { + it( 'should not loop', async () => { + const user = userEvent.setup(); + + render( + + ); + + await user.tab(); + expect( getOption( 'Option One' ) ).toHaveFocus(); + await user.keyboard( '[ArrowRight]' ); + expect( getOption( 'Option Two' ) ).toHaveFocus(); + await user.keyboard( '[ArrowRight]' ); + expect( getOption( 'Option Two' ) ).toHaveFocus(); + } ); + } ); +} ); diff --git a/packages/components/src/circular-option-picker/types.ts b/packages/components/src/circular-option-picker/types.ts index 7ebd78da4020f3..8788c2edcd2051 100644 --- a/packages/components/src/circular-option-picker/types.ts +++ b/packages/components/src/circular-option-picker/types.ts @@ -15,7 +15,7 @@ import type { ButtonAsButtonProps } from '../button/types'; import type { DropdownProps } from '../dropdown/types'; import type { WordPressComponentProps } from '../ui/context'; -export type CircularOptionPickerProps = { +type CommonCircularOptionPickerProps = { /** * An ID to apply to the component. */ @@ -41,21 +41,40 @@ export type CircularOptionPickerProps = { */ children?: ReactNode; /** - * Whether the keyboard interaction should wrap around. + * Whether the control should present as a set of buttons, + * each with its own tab stop. * - * @default true + * @default false */ - loop?: boolean; -} & ( - | { - 'aria-label': string; - 'aria-labelledby'?: never; - } - | { - 'aria-label'?: never; - 'aria-labelledby': string; - } - ); + asButtons?: boolean; +}; + +export type ListboxCircularOptionPickerProps = + CommonCircularOptionPickerProps & { + asButtons: false | undefined; + /** + * Prevents keyboard interaction from wrapping around. + * Only used when `asButtons` is not true. + * + * @default false + */ + disableLooping?: boolean; + } & ( + | { + 'aria-label': string; + 'aria-labelledby'?: never; + } + | { + 'aria-label'?: never; + 'aria-labelledby': string; + } + ); + +export type ButtonsCircularOptionPickerProps = CommonCircularOptionPickerProps; + +export type CircularOptionPickerProps = + | ListboxCircularOptionPickerProps + | ButtonsCircularOptionPickerProps; export type DropdownLinkActionProps = { buttonProps?: Omit< From a5ef383f6a79bd9fa751c07909309971ae46ca54 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Fri, 8 Sep 2023 09:42:49 +0100 Subject: [PATCH 02/25] Updating `CircularOptionPicker` consumers They all need to pass through the new `asButtons`/`disableLooping` prop following the `CircularOptionPicker` refactor. --- .../components/src/color-palette/index.tsx | 29 +++++++++++----- .../components/src/color-palette/types.ts | 14 ++++++++ .../src/duotone-picker/duotone-picker.tsx | 21 +++++++++--- .../components/src/duotone-picker/types.ts | 14 ++++++++ .../components/src/gradient-picker/index.tsx | 33 ++++++++++++++----- .../components/src/gradient-picker/types.ts | 14 ++++++++ 6 files changed, 104 insertions(+), 21 deletions(-) diff --git a/packages/components/src/color-palette/index.tsx b/packages/components/src/color-palette/index.tsx index 72cd4c0f171ae5..08a5b5ce50e952 100644 --- a/packages/components/src/color-palette/index.tsx +++ b/packages/components/src/color-palette/index.tsx @@ -48,7 +48,7 @@ function SinglePalette( { colors, onChange, value, - ...otherProps + ...additionalProps }: SinglePaletteProps ) { const colorOptions = useMemo( () => { return colors.map( ( { color, name }, index ) => { @@ -95,7 +95,7 @@ function SinglePalette( { ); } @@ -178,6 +178,8 @@ function UnforwardedColorPalette( forwardedRef: ForwardedRef< any > ) { const { + asButtons, + disableLooping, clearable = true, colors = [], disableCustomColors = false, @@ -188,7 +190,7 @@ function UnforwardedColorPalette( headingLevel = 2, 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledby, - ...otherProps + ...additionalProps } = props; const [ normalizedColorValue, setNormalizedColorValue ] = useState( value ); @@ -248,17 +250,27 @@ function UnforwardedColorPalette( ); - let ariaProps: { 'aria-label': string } | { 'aria-labelledby': string }; + let ariaProps: + | { 'aria-label': string } + | { 'aria-labelledby': string } + | {} = {}; + if ( ariaLabel ) { ariaProps = { 'aria-label': ariaLabel }; } else if ( ariaLabelledby ) { - ariaProps = { 'aria-labelledby': ariaLabelledby }; - } else { - ariaProps = { 'aria-label': __( 'Custom color picker.' ) }; + ariaProps = { + 'aria-labelledby': ariaLabelledby, + }; + } else if ( ! asButtons ) { + ariaProps = { + 'aria-label': __( 'Custom color picker.' ), + }; } + const metaProps = asButtons ? { asButtons } : { disableLooping }; + return ( - + { ! disableCustomColors && ( & { * Currently active value. */ value?: string; + /** + * Whether the control should present as a set of buttons, + * each with its own tab stop. + * + * @default false + */ + asButtons?: boolean; + /** + * Prevents keyboard interaction from wrapping around. + * Only used when `asButtons` is not true. + * + * @default false + */ + disableLooping?: boolean; /** * Whether this is rendered in the sidebar. * diff --git a/packages/components/src/duotone-picker/duotone-picker.tsx b/packages/components/src/duotone-picker/duotone-picker.tsx index 7a681f67bec8b7..ab5f9f0bcdd036 100644 --- a/packages/components/src/duotone-picker/duotone-picker.tsx +++ b/packages/components/src/duotone-picker/duotone-picker.tsx @@ -55,6 +55,8 @@ import type { DuotonePickerProps } from './types'; * ``` */ function DuotonePicker( { + asButtons, + disableLooping, clearable = true, unsetable = true, colorPalette, @@ -122,18 +124,29 @@ function DuotonePicker( { ); } ); - let ariaProps: { 'aria-label': string } | { 'aria-labelledby': string }; + let ariaProps: + | { 'aria-label': string } + | { 'aria-labelledby': string } + | {} = {}; + if ( ariaLabel ) { ariaProps = { 'aria-label': ariaLabel }; } else if ( ariaLabelledby ) { - ariaProps = { 'aria-labelledby': ariaLabelledby }; - } else { - ariaProps = { 'aria-label': __( 'Custom color picker.' ) }; + ariaProps = { + 'aria-labelledby': ariaLabelledby, + }; + } else if ( ! asButtons ) { + ariaProps = { + 'aria-label': __( 'Custom color picker.' ), + }; } + const metaProps = asButtons ? { asButtons } : { disableLooping }; + return ( void; + /** + * Whether the control should present as a set of buttons, + * each with its own tab stop. + * + * @default false + */ + asButtons?: boolean; + /** + * Prevents keyboard interaction from wrapping around. + * Only used when `asButtons` is not true. + * + * @default false + */ + disableLooping?: boolean; } & ( | { /** diff --git a/packages/components/src/gradient-picker/index.tsx b/packages/components/src/gradient-picker/index.tsx index e384d62925bcb4..128e5c085fb0c2 100644 --- a/packages/components/src/gradient-picker/index.tsx +++ b/packages/components/src/gradient-picker/index.tsx @@ -41,7 +41,7 @@ function SingleOrigin( { gradients, onChange, value, - ...otherProps + ...additionalProps }: PickerProps< GradientObject > ) { const gradientOptions = useMemo( () => { return gradients.map( ( { gradient, name }, index ) => ( @@ -74,7 +74,7 @@ function SingleOrigin( { ); } @@ -116,30 +116,43 @@ function MultipleOrigin( { function Component( props: PickerProps< any > ) { const { + asButtons, + disableLooping, actions, headingLevel, 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledby, - ...otherProps + ...additionalProps } = props; const options = isMultipleOriginArray( props.gradients ) ? ( - + ) : ( - + ); - let ariaProps: { 'aria-label': string } | { 'aria-labelledby': string }; + let ariaProps: + | { 'aria-label': string } + | { 'aria-labelledby': string } + | {} = {}; + if ( ariaLabel ) { ariaProps = { 'aria-label': ariaLabel }; } else if ( ariaLabelledby ) { - ariaProps = { 'aria-labelledby': ariaLabelledby }; - } else { - ariaProps = { 'aria-label': __( 'Custom gradient picker.' ) }; + ariaProps = { + 'aria-labelledby': ariaLabelledby, + }; + } else if ( ! asButtons ) { + ariaProps = { + 'aria-label': __( 'Custom gradient picker.' ), + }; } + const metaProps = asButtons ? { asButtons } : { disableLooping }; + return ( @@ -200,6 +213,7 @@ export function GradientPicker( { disableCustomGradients = false, __experimentalIsRenderedInSidebar, headingLevel = 2, + ...additionalProps }: GradientPickerComponentProps ) { const clearGradient = useCallback( () => onChange( undefined ), @@ -237,6 +251,7 @@ export function GradientPicker( { ) } { ( gradients.length || clearable ) && ( Date: Fri, 8 Sep 2023 14:16:13 +0100 Subject: [PATCH 03/25] Updating CHANGELOG.md --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index ca2d278909aa78..0b035d472e32a7 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -12,6 +12,7 @@ - `ToggleGroupControl`: Rewrite backdrop animation using framer motion shared layout animations, add better support for controlled and uncontrolled modes ([#50278](https://github.com/WordPress/gutenberg/pull/50278)). - `Popover`: Add the `is-positioned` CSS class only after the popover has finished animating ([#54178](https://github.com/WordPress/gutenberg/pull/54178)). - `Tooltip`: Replace the existing tooltip to simplify the implementation and improve accessibility while maintaining the same behaviors and API ([#48440](https://github.com/WordPress/gutenberg/pull/48440)). +- `CircularOptionPicker`: Add option to use previous non-listbox behaviour, for contexts where buttons are more appropriate than a list of options ([#54290](https://github.com/WordPress/gutenberg/pull/54290)). ### Bug Fix From 8de887563ffdb9e4824b15375214cc95865813b5 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Fri, 8 Sep 2023 16:19:36 +0100 Subject: [PATCH 04/25] Fixing circular dependency --- .../components/src/circular-option-picker/option-picker.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/components/src/circular-option-picker/option-picker.tsx b/packages/components/src/circular-option-picker/option-picker.tsx index 25f7899f79f223..89419e1d21cc12 100644 --- a/packages/components/src/circular-option-picker/option-picker.tsx +++ b/packages/components/src/circular-option-picker/option-picker.tsx @@ -90,7 +90,11 @@ function ListboxCircularOptionPicker( compositeState.setBaseId( baseId ); compositeState.setLoop( !! loop ); compositeState.setRTL( rtl ); - }, [ compositeState, baseId, loop, rtl ] ); + // Disabling exhaustive-deps check because it expects + // `compositeState` to be present, but doing so causes + // an infinite loop. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [ baseId, loop, rtl ] ); return ( Date: Tue, 12 Sep 2023 17:09:12 +0100 Subject: [PATCH 05/25] Updating subcomponent filenames --- ...actions.tsx => circular-option-picker-actions.tsx} | 0 ...context.tsx => circular-option-picker-context.tsx} | 0 ...up.tsx => circular-option-picker-option-group.tsx} | 0 ...r-option.tsx => circular-option-picker-option.tsx} | 2 +- .../{option-picker.tsx => circular-option-picker.tsx} | 11 +++++++---- .../components/src/circular-option-picker/index.tsx | 11 +++++++---- 6 files changed, 15 insertions(+), 9 deletions(-) rename packages/components/src/circular-option-picker/{option-picker-actions.tsx => circular-option-picker-actions.tsx} (100%) rename packages/components/src/circular-option-picker/{option-picker-context.tsx => circular-option-picker-context.tsx} (100%) rename packages/components/src/circular-option-picker/{option-picker-option-group.tsx => circular-option-picker-option-group.tsx} (100%) rename packages/components/src/circular-option-picker/{option-picker-option.tsx => circular-option-picker-option.tsx} (97%) rename packages/components/src/circular-option-picker/{option-picker.tsx => circular-option-picker.tsx} (93%) diff --git a/packages/components/src/circular-option-picker/option-picker-actions.tsx b/packages/components/src/circular-option-picker/circular-option-picker-actions.tsx similarity index 100% rename from packages/components/src/circular-option-picker/option-picker-actions.tsx rename to packages/components/src/circular-option-picker/circular-option-picker-actions.tsx diff --git a/packages/components/src/circular-option-picker/option-picker-context.tsx b/packages/components/src/circular-option-picker/circular-option-picker-context.tsx similarity index 100% rename from packages/components/src/circular-option-picker/option-picker-context.tsx rename to packages/components/src/circular-option-picker/circular-option-picker-context.tsx diff --git a/packages/components/src/circular-option-picker/option-picker-option-group.tsx b/packages/components/src/circular-option-picker/circular-option-picker-option-group.tsx similarity index 100% rename from packages/components/src/circular-option-picker/option-picker-option-group.tsx rename to packages/components/src/circular-option-picker/circular-option-picker-option-group.tsx diff --git a/packages/components/src/circular-option-picker/option-picker-option.tsx b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx similarity index 97% rename from packages/components/src/circular-option-picker/option-picker-option.tsx rename to packages/components/src/circular-option-picker/circular-option-picker-option.tsx index 863f24a1726c86..0d2fbe746afb7b 100644 --- a/packages/components/src/circular-option-picker/option-picker-option.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx @@ -13,7 +13,7 @@ import { Icon, check } from '@wordpress/icons'; /** * Internal dependencies */ -import { CircularOptionPickerContext } from './option-picker-context'; +import { CircularOptionPickerContext } from './circular-option-picker-context'; import Button from '../button'; import { CompositeItem } from '../composite'; import Tooltip from '../tooltip'; diff --git a/packages/components/src/circular-option-picker/option-picker.tsx b/packages/components/src/circular-option-picker/circular-option-picker.tsx similarity index 93% rename from packages/components/src/circular-option-picker/option-picker.tsx rename to packages/components/src/circular-option-picker/circular-option-picker.tsx index 89419e1d21cc12..3cc654711cec60 100644 --- a/packages/components/src/circular-option-picker/option-picker.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker.tsx @@ -13,16 +13,19 @@ import { isRTL } from '@wordpress/i18n'; /** * Internal dependencies */ -import { CircularOptionPickerContext } from './option-picker-context'; +import { CircularOptionPickerContext } from './circular-option-picker-context'; import { Composite, useCompositeState } from '../composite'; import type { CircularOptionPickerProps, ListboxCircularOptionPickerProps, ButtonsCircularOptionPickerProps, } from './types'; -import { Option } from './option-picker-option'; -import { OptionGroup } from './option-picker-option-group'; -import { ButtonAction, DropdownLinkAction } from './option-picker-actions'; +import { Option } from './circular-option-picker-option'; +import { OptionGroup } from './circular-option-picker-option-group'; +import { + ButtonAction, + DropdownLinkAction, +} from './circular-option-picker-actions'; /** *`CircularOptionPicker` is a component that displays a set of options as circular buttons. diff --git a/packages/components/src/circular-option-picker/index.tsx b/packages/components/src/circular-option-picker/index.tsx index d58ee44430dd4d..ef975c21ee6545 100644 --- a/packages/components/src/circular-option-picker/index.tsx +++ b/packages/components/src/circular-option-picker/index.tsx @@ -1,10 +1,13 @@ /** * Internal dependencies */ -import CircularOptionPicker from './option-picker'; +import CircularOptionPicker from './circular-option-picker'; -export { Option } from './option-picker-option'; -export { OptionGroup } from './option-picker-option-group'; -export { ButtonAction, DropdownLinkAction } from './option-picker-actions'; +export { Option } from './circular-option-picker-option'; +export { OptionGroup } from './circular-option-picker-option-group'; +export { + ButtonAction, + DropdownLinkAction, +} from './circular-option-picker-actions'; export default CircularOptionPicker; From 954932bccf9ea708481da4bc353bfa22c6b8b222 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Wed, 13 Sep 2023 09:30:52 +0100 Subject: [PATCH 06/25] Simplifying `selectedIconProps` logic --- .../circular-option-picker-option.tsx | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx index 0d2fbe746afb7b..606d018a1ad513 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx @@ -53,7 +53,7 @@ function renderOptionAsOption( props: { export function Option( { className, isSelected, - selectedIconProps, + selectedIconProps = {}, tooltipText, ...additionalProps }: OptionProps ) { @@ -107,12 +107,7 @@ export function Option( { ) : ( optionControl ) } - { isSelected && ( - - ) } + { isSelected && } ); } From e3ab6d286859b5fcd695fa0f3cd76308e01a1733 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Wed, 13 Sep 2023 09:44:13 +0100 Subject: [PATCH 07/25] Refactoring `commonProps` --- .../circular-option-picker-option.tsx | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx index 606d018a1ad513..7ee9113c7d3a15 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx @@ -80,19 +80,21 @@ export function Option( { } }, [ baseId, currentId, id, isComposite, isSelected, setCurrentId ] ); + const commonProps = { + id, + className: 'components-circular-option-picker__option', + ...additionalProps, + }; + const optionControl = isComposite ? renderOptionAsOption( { - id, - className: 'components-circular-option-picker__option', + ...commonProps, isSelected, compositeState, - ...additionalProps, } ) : renderOptionAsButton( { - id, - className: 'components-circular-option-picker__option', + ...commonProps, isPressed: isSelected, - ...additionalProps, } ); return ( From 61c676264dca2529bb3dddc4796c722167933aad Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Wed, 13 Sep 2023 09:52:47 +0100 Subject: [PATCH 08/25] Refactoring render functions into components --- .../circular-option-picker-option.tsx | 62 +++++++++---------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx index 7ee9113c7d3a15..b4827839b5a722 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx @@ -21,26 +21,43 @@ import type { OptionProps } from './types'; const hasSelectedOption = new Map(); -function renderOptionAsButton( props: { +function OptionAsButton( props: { + id?: string; className?: string; isPressed?: boolean; } ) { return ; } -function renderOptionAsOption( props: { +function OptionAsOption( props: { id: string; className?: string; isSelected?: boolean; compositeState: any; } ) { - const { className, isSelected, compositeState, ...additionalProps } = props; + const { id, className, isSelected, compositeState, ...additionalProps } = + props; + const { baseId, currentId, setCurrentId } = compositeState as any; + + useEffect( () => { + // If we call `setCurrentId` here, it doesn't update for other + // Option renders in the same pass. So we have to store our own + // map to make sure that we only set the first selected option. + // We still need to check `currentId` because the control will + // update this as the user moves around, and that state should + // be maintained as the group gains and loses focus. + if ( isSelected && ! currentId && ! hasSelectedOption.get( baseId ) ) { + hasSelectedOption.set( baseId, true ); + setCurrentId( id ); + } + }, [ baseId, currentId, id, isSelected, setCurrentId ] ); return ( { - // If we call `setCurrentId` here, it doesn't update for other - // Option renders in the same pass. So we have to store our own - // map to make sure that we only set the first selected option. - // We still need to check `currentId` because the control will - // update this as the user moves around, and that state should - // be maintained as the group gains and loses focus. - if ( - isComposite && - isSelected && - ! currentId && - ! hasSelectedOption.get( baseId ) - ) { - hasSelectedOption.set( baseId, true ); - setCurrentId( id ); - } - }, [ baseId, currentId, id, isComposite, isSelected, setCurrentId ] ); - const commonProps = { id, className: 'components-circular-option-picker__option', ...additionalProps, }; - const optionControl = isComposite - ? renderOptionAsOption( { - ...commonProps, - isSelected, - compositeState, - } ) - : renderOptionAsButton( { - ...commonProps, - isPressed: isSelected, - } ); + const optionControl = isComposite ? ( + + ) : ( + + ); return (
Date: Wed, 13 Sep 2023 10:08:59 +0100 Subject: [PATCH 09/25] Letting `Button` deal with classes --- .../circular-option-picker-option.tsx | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx index b4827839b5a722..d8474ab3d3b6fb 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx @@ -35,8 +35,7 @@ function OptionAsOption( props: { isSelected?: boolean; compositeState: any; } ) { - const { id, className, isSelected, compositeState, ...additionalProps } = - props; + const { id, isSelected, compositeState, ...additionalProps } = props; const { baseId, currentId, setCurrentId } = compositeState as any; useEffect( () => { @@ -58,11 +57,10 @@ function OptionAsOption( props: { { ...compositeState } as={ Button } id={ id } - className={ classnames( className, { - 'is-pressed': isSelected, - } ) } role="option" + isPressed={ isSelected } aria-selected={ !! isSelected } + aria-pressed={ null } /> ); } From b446436ee81482cb083467ed045617a783edca7e Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Wed, 13 Sep 2023 11:02:39 +0100 Subject: [PATCH 10/25] Splitting state props into multiple effects --- .../circular-option-picker.tsx | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker.tsx b/packages/components/src/circular-option-picker/circular-option-picker.tsx index 3cc654711cec60..d9a8ca78c94c23 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker.tsx @@ -86,18 +86,27 @@ function ListboxCircularOptionPicker( const compositeState = useCompositeState( { baseId, loop, rtl } ); - // This is necessary as `useCompositeState` is sealed after + // These are necessary as `useCompositeState` is sealed after // the first render, so although unlikely to happen, if a state // property should change, we need to process it accordingly. + useEffect( () => { compositeState.setBaseId( baseId ); - compositeState.setLoop( !! loop ); - compositeState.setRTL( rtl ); // Disabling exhaustive-deps check because it expects // `compositeState` to be present, but doing so causes // an infinite loop. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ baseId, loop, rtl ] ); + }, [ baseId ] ); + + useEffect( () => { + compositeState.setLoop( !! loop ); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [ loop ] ); + + useEffect( () => { + compositeState.setRTL( rtl ); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [ rtl ] ); return ( Date: Wed, 13 Sep 2023 13:38:04 +0100 Subject: [PATCH 11/25] Reverting `disableLooping` prop to `loop` --- .../circular-option-picker/circular-option-picker.tsx | 3 +-- .../circular-option-picker/stories/index.story.tsx | 11 ++++++++++- .../src/circular-option-picker/test/index.tsx | 10 +++++----- .../components/src/circular-option-picker/types.ts | 6 +++--- packages/components/src/color-palette/index.tsx | 4 ++-- packages/components/src/color-palette/types.ts | 4 ++-- .../components/src/duotone-picker/duotone-picker.tsx | 4 ++-- packages/components/src/duotone-picker/types.ts | 4 ++-- packages/components/src/gradient-picker/index.tsx | 4 ++-- packages/components/src/gradient-picker/types.ts | 4 ++-- 10 files changed, 31 insertions(+), 23 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker.tsx b/packages/components/src/circular-option-picker/circular-option-picker.tsx index d9a8ca78c94c23..a42c7c84b29d93 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker.tsx @@ -79,10 +79,9 @@ function ListboxCircularOptionPicker( 'asButtons' | 'actions' | 'options' > ) { - const { id, disableLooping, children, ...additionalProps } = props; + const { id, loop = true, children, ...additionalProps } = props; const baseId = useInstanceId( CircularOptionPicker, 'option-picker', id ); const rtl = isRTL(); - const loop = ! disableLooping; const compositeState = useCompositeState( { baseId, loop, rtl } ); diff --git a/packages/components/src/circular-option-picker/stories/index.story.tsx b/packages/components/src/circular-option-picker/stories/index.story.tsx index c05a284340e997..20c3f543bb751c 100644 --- a/packages/components/src/circular-option-picker/stories/index.story.tsx +++ b/packages/components/src/circular-option-picker/stories/index.story.tsx @@ -111,7 +111,10 @@ const Template: StoryFn< typeof CircularOptionPicker > = ( props ) => ( ); export const Default = Template.bind( {} ); -Default.args = { options: }; +Default.args = { + 'aria-label': 'Circular Option Picker', + options: , +}; export const AsButtons = Template.bind( {} ); AsButtons.args = { @@ -119,6 +122,12 @@ AsButtons.args = { asButtons: true, }; +export const WithLoopingDisabled = Template.bind( {} ); +WithLoopingDisabled.args = { + ...Default.args, + loop: false, +}; + export const WithButtonAction = Template.bind( {} ); WithButtonAction.args = { ...Default.args, diff --git a/packages/components/src/circular-option-picker/test/index.tsx b/packages/components/src/circular-option-picker/test/index.tsx index 7ba03183d835fe..48a0aca40faf3e 100644 --- a/packages/components/src/circular-option-picker/test/index.tsx +++ b/packages/components/src/circular-option-picker/test/index.tsx @@ -69,7 +69,7 @@ describe( 'CircularOptionPicker', () => { } ); } ); - describe( 'when `disableLooping` is not set', () => { + describe( 'when `loop` is not set', () => { it( 'should loop', async () => { const user = userEvent.setup(); @@ -89,7 +89,7 @@ describe( 'CircularOptionPicker', () => { } ); } ); - describe( 'when `disableLooping` is false', () => { + describe( 'when `loop` is true', () => { it( 'should loop', async () => { const user = userEvent.setup(); @@ -97,7 +97,7 @@ describe( 'CircularOptionPicker', () => { ); @@ -110,14 +110,14 @@ describe( 'CircularOptionPicker', () => { } ); } ); - describe( 'when `disableLooping` is true', () => { + describe( 'when `loop` is false', () => { it( 'should not loop', async () => { const user = userEvent.setup(); render( ); diff --git a/packages/components/src/circular-option-picker/types.ts b/packages/components/src/circular-option-picker/types.ts index 8788c2edcd2051..ef2cf4943b2901 100644 --- a/packages/components/src/circular-option-picker/types.ts +++ b/packages/components/src/circular-option-picker/types.ts @@ -51,14 +51,14 @@ type CommonCircularOptionPickerProps = { export type ListboxCircularOptionPickerProps = CommonCircularOptionPickerProps & { - asButtons: false | undefined; + asButtons?: false; /** * Prevents keyboard interaction from wrapping around. * Only used when `asButtons` is not true. * - * @default false + * @default true */ - disableLooping?: boolean; + loop?: boolean; } & ( | { 'aria-label': string; diff --git a/packages/components/src/color-palette/index.tsx b/packages/components/src/color-palette/index.tsx index 08a5b5ce50e952..fca732adc7052d 100644 --- a/packages/components/src/color-palette/index.tsx +++ b/packages/components/src/color-palette/index.tsx @@ -179,7 +179,7 @@ function UnforwardedColorPalette( ) { const { asButtons, - disableLooping, + loop, clearable = true, colors = [], disableCustomColors = false, @@ -267,7 +267,7 @@ function UnforwardedColorPalette( }; } - const metaProps = asButtons ? { asButtons } : { disableLooping }; + const metaProps = asButtons ? { asButtons } : { loop }; return ( diff --git a/packages/components/src/color-palette/types.ts b/packages/components/src/color-palette/types.ts index 325f01b6ac4acb..0b097aa108d38e 100644 --- a/packages/components/src/color-palette/types.ts +++ b/packages/components/src/color-palette/types.ts @@ -93,9 +93,9 @@ export type ColorPaletteProps = Pick< PaletteProps, 'onChange' > & { * Prevents keyboard interaction from wrapping around. * Only used when `asButtons` is not true. * - * @default false + * @default true */ - disableLooping?: boolean; + loop?: boolean; /** * Whether this is rendered in the sidebar. * diff --git a/packages/components/src/duotone-picker/duotone-picker.tsx b/packages/components/src/duotone-picker/duotone-picker.tsx index ab5f9f0bcdd036..01c7fc9bc33a3d 100644 --- a/packages/components/src/duotone-picker/duotone-picker.tsx +++ b/packages/components/src/duotone-picker/duotone-picker.tsx @@ -56,7 +56,7 @@ import type { DuotonePickerProps } from './types'; */ function DuotonePicker( { asButtons, - disableLooping, + loop, clearable = true, unsetable = true, colorPalette, @@ -141,7 +141,7 @@ function DuotonePicker( { }; } - const metaProps = asButtons ? { asButtons } : { disableLooping }; + const metaProps = asButtons ? { asButtons } : { loop }; return ( ) { const { asButtons, - disableLooping, + loop, actions, headingLevel, 'aria-label': ariaLabel, @@ -147,7 +147,7 @@ function Component( props: PickerProps< any > ) { }; } - const metaProps = asButtons ? { asButtons } : { disableLooping }; + const metaProps = asButtons ? { asButtons } : { loop }; return ( Date: Wed, 13 Sep 2023 13:41:33 +0100 Subject: [PATCH 12/25] Adding note explaining `aria-pressed={null}` --- .../circular-option-picker/circular-option-picker-option.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx index d8474ab3d3b6fb..6f6a935c7b6e99 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx @@ -60,6 +60,10 @@ function OptionAsOption( props: { role="option" isPressed={ isSelected } aria-selected={ !! isSelected } + // `Button` sets `aria-pressed` as standard, based + // on `isPressed`. However, `role="option"` uses + // `aria-selected` instead, so we have to explicitly + // remove it as an attribute here. aria-pressed={ null } /> ); From 96acbc5b46a52e6363393e57c4c03ed9a3031a25 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Wed, 13 Sep 2023 15:41:28 +0100 Subject: [PATCH 13/25] Fixing some typing, removing `any` --- .../circular-option-picker-context.tsx | 8 ++++- .../circular-option-picker-option-group.tsx | 7 +++-- .../circular-option-picker-option.tsx | 28 +++++++++++------- .../circular-option-picker.tsx | 29 +++++++++++++------ .../src/circular-option-picker/types.ts | 6 ++++ packages/components/src/composite/index.ts | 3 ++ 6 files changed, 58 insertions(+), 23 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker-context.tsx b/packages/components/src/circular-option-picker/circular-option-picker-context.tsx index b66a386471863e..cafb0b90e3017f 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker-context.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker-context.tsx @@ -3,4 +3,10 @@ */ import { createContext } from '@wordpress/element'; -export const CircularOptionPickerContext = createContext( {} ); +/** + * Internal dependencies + */ +import type { CircularOptionPickerContextProps } from './types'; + +export const CircularOptionPickerContext = + createContext< CircularOptionPickerContextProps >( {} ); diff --git a/packages/components/src/circular-option-picker/circular-option-picker-option-group.tsx b/packages/components/src/circular-option-picker/circular-option-picker-option-group.tsx index b7ac44c1c01991..2b50d80799b849 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker-option-group.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker-option-group.tsx @@ -13,9 +13,10 @@ export function OptionGroup( { options, ...additionalProps }: OptionGroupProps ) { - const { 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledby } = - additionalProps as any; - const role = ariaLabel || ariaLabelledby ? 'group' : undefined; + const role = + 'aria-label' in additionalProps || 'aria-labelledby' in additionalProps + ? 'group' + : undefined; return (
{ // If we call `setCurrentId` here, it doesn't update for other @@ -64,7 +71,6 @@ function OptionAsOption( props: { // on `isPressed`. However, `role="option"` uses // `aria-selected` instead, so we have to explicitly // remove it as an attribute here. - aria-pressed={ null } /> ); } @@ -76,10 +82,12 @@ export function Option( { tooltipText, ...additionalProps }: OptionProps ) { - const compositeState = useContext( CircularOptionPickerContext ); - const { baseId } = compositeState as any; - const isComposite = !! baseId; - const id = useInstanceId( Option, baseId ); + const compositeContext = useContext( CircularOptionPickerContext ); + const { isComposite, baseId } = compositeContext; + const id = useInstanceId( + Option, + baseId || 'components-circular-option-picker__option' + ); const commonProps = { id, @@ -90,7 +98,7 @@ export function Option( { const optionControl = isComposite ? ( ) : ( diff --git a/packages/components/src/circular-option-picker/circular-option-picker.tsx b/packages/components/src/circular-option-picker/circular-option-picker.tsx index a42c7c84b29d93..ba5d67be1652af 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker.tsx @@ -77,10 +77,9 @@ function ListboxCircularOptionPicker( props: Omit< ListboxCircularOptionPickerProps, 'asButtons' | 'actions' | 'options' - > + > & { baseId: string } ) { - const { id, loop = true, children, ...additionalProps } = props; - const baseId = useInstanceId( CircularOptionPicker, 'option-picker', id ); + const { baseId, loop = true, children, ...additionalProps } = props; const rtl = isRTL(); const compositeState = useCompositeState( { baseId, loop, rtl } ); @@ -107,13 +106,18 @@ function ListboxCircularOptionPicker( // eslint-disable-next-line react-hooks/exhaustive-deps }, [ rtl ] ); + const compositeContext = { + isComposite: true, + ...compositeState, + }; + return ( - + { children } @@ -124,15 +128,15 @@ function ButtonsCircularOptionPicker( props: Omit< ButtonsCircularOptionPickerProps, 'asButtons' | 'actions' | 'options' - > + > & { baseId: string } ) { - const { children, ...additionalProps } = props; + const { children, baseId, ...additionalProps } = props; - // We're including an empty context here, so as to prevent - // any nesting issues. return (
- + { children }
@@ -149,6 +153,12 @@ function CircularOptionPicker( props: CircularOptionPickerProps ) { ...additionalProps } = props; + const baseId = useInstanceId( + CircularOptionPicker, + 'components-circular-option-picker', + additionalProps.id + ); + const OptionPickerImplementation = asButtons ? ButtonsCircularOptionPicker : ListboxCircularOptionPicker; @@ -156,6 +166,7 @@ function CircularOptionPicker( props: CircularOptionPickerProps ) { return ( ; }; + +export type CircularOptionPickerCompositeState = CompositeState; +export type CircularOptionPickerContextProps = + | { isComposite?: false; baseId?: string } + | ( { isComposite: true } & CircularOptionPickerCompositeState ); diff --git a/packages/components/src/composite/index.ts b/packages/components/src/composite/index.ts index ddb10fffa12ad9..2a4e3655426908 100644 --- a/packages/components/src/composite/index.ts +++ b/packages/components/src/composite/index.ts @@ -17,3 +17,6 @@ export { CompositeItem, useCompositeState, } from 'reakit'; + +/* eslint-disable-next-line no-restricted-imports */ +export type { CompositeStateReturn as CompositeState } from 'reakit'; From 08dca39a04956a0b5f53290cd75f8ff3a896a904 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Wed, 13 Sep 2023 15:54:48 +0100 Subject: [PATCH 14/25] Reverting back to manual `is-pressed` style --- .../circular-option-picker-option.tsx | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx index afbd1e0b406bad..62e584bcfb67ff 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx @@ -39,7 +39,7 @@ function OptionAsOption( props: { isSelected?: boolean; context: CircularOptionPickerContextProps; } ) { - const { id, isSelected, context, ...additionalProps } = props; + const { id, className, isSelected, context, ...additionalProps } = props; const { isComposite, ..._compositeState } = context; const compositeState = _compositeState as CircularOptionPickerCompositeState; @@ -64,13 +64,11 @@ function OptionAsOption( props: { { ...compositeState } as={ Button } id={ id } + className={ classnames( className, { + 'is-pressed': isSelected, + } ) } role="option" - isPressed={ isSelected } aria-selected={ !! isSelected } - // `Button` sets `aria-pressed` as standard, based - // on `isPressed`. However, `role="option"` uses - // `aria-selected` instead, so we have to explicitly - // remove it as an attribute here. /> ); } From eb916e06304c26642d3ce51bfe642f17561493ea Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Wed, 13 Sep 2023 23:43:46 +0100 Subject: [PATCH 15/25] Refactoring `Omit` --- .../circular-option-picker.tsx | 10 +--- .../src/circular-option-picker/types.ts | 60 ++++++++++++------- .../components/src/color-palette/index.tsx | 40 ++++++++----- .../src/duotone-picker/duotone-picker.tsx | 40 ++++++++----- .../components/src/gradient-picker/index.tsx | 40 ++++++++----- 5 files changed, 111 insertions(+), 79 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker.tsx b/packages/components/src/circular-option-picker/circular-option-picker.tsx index ba5d67be1652af..b46c83ec47900c 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker.tsx @@ -74,10 +74,7 @@ import { */ function ListboxCircularOptionPicker( - props: Omit< - ListboxCircularOptionPickerProps, - 'asButtons' | 'actions' | 'options' - > & { baseId: string } + props: ListboxCircularOptionPickerProps ) { const { baseId, loop = true, children, ...additionalProps } = props; const rtl = isRTL(); @@ -125,10 +122,7 @@ function ListboxCircularOptionPicker( } function ButtonsCircularOptionPicker( - props: Omit< - ButtonsCircularOptionPickerProps, - 'asButtons' | 'actions' | 'options' - > & { baseId: string } + props: ButtonsCircularOptionPickerProps ) { const { children, baseId, ...additionalProps } = props; diff --git a/packages/components/src/circular-option-picker/types.ts b/packages/components/src/circular-option-picker/types.ts index 22156d9add51ec..2c2b92ba097e54 100644 --- a/packages/components/src/circular-option-picker/types.ts +++ b/packages/components/src/circular-option-picker/types.ts @@ -50,32 +50,46 @@ type CommonCircularOptionPickerProps = { asButtons?: boolean; }; -export type ListboxCircularOptionPickerProps = - CommonCircularOptionPickerProps & { - asButtons?: false; - /** - * Prevents keyboard interaction from wrapping around. - * Only used when `asButtons` is not true. - * - * @default true - */ - loop?: boolean; - } & ( - | { - 'aria-label': string; - 'aria-labelledby'?: never; - } - | { - 'aria-label'?: never; - 'aria-labelledby': string; - } - ); +type WithBaseId = { + baseId: string; +}; + +type FullListboxCircularOptionPickerProps = CommonCircularOptionPickerProps & { + /** + * Prevents keyboard interaction from wrapping around. + * Only used when `asButtons` is not true. + * + * @default true + */ + loop?: boolean; +} & ( + | { + 'aria-label': string; + 'aria-labelledby'?: never; + } + | { + 'aria-label'?: never; + 'aria-labelledby': string; + } + ); -export type ButtonsCircularOptionPickerProps = CommonCircularOptionPickerProps; +export type ListboxCircularOptionPickerProps = WithBaseId & + Omit< + FullListboxCircularOptionPickerProps, + 'asButtons' | 'actions' | 'options' + >; + +type FullButtonsCircularOptionPickerProps = CommonCircularOptionPickerProps; + +export type ButtonsCircularOptionPickerProps = WithBaseId & + Omit< + FullButtonsCircularOptionPickerProps, + 'asButtons' | 'actions' | 'options' + >; export type CircularOptionPickerProps = - | ListboxCircularOptionPickerProps - | ButtonsCircularOptionPickerProps; + | ( { asButtons?: false } & FullListboxCircularOptionPickerProps ) + | ( { asButtons: true } & FullButtonsCircularOptionPickerProps ); export type DropdownLinkActionProps = { buttonProps?: Omit< diff --git a/packages/components/src/color-palette/index.tsx b/packages/components/src/color-palette/index.tsx index fca732adc7052d..60aeb6ed4d39ed 100644 --- a/packages/components/src/color-palette/index.tsx +++ b/packages/components/src/color-palette/index.tsx @@ -250,24 +250,33 @@ function UnforwardedColorPalette( ); - let ariaProps: - | { 'aria-label': string } - | { 'aria-labelledby': string } - | {} = {}; + let metaProps: + | { asButtons: false; loop?: boolean; 'aria-label': string } + | { asButtons: false; loop?: boolean; 'aria-labelledby': string } + | { asButtons: true }; - if ( ariaLabel ) { - ariaProps = { 'aria-label': ariaLabel }; - } else if ( ariaLabelledby ) { - ariaProps = { - 'aria-labelledby': ariaLabelledby, + if ( asButtons ) { + metaProps = { asButtons: true }; + } else { + const _metaProps: { asButtons: false; loop?: boolean } = { + asButtons: false, + loop, }; - } else if ( ! asButtons ) { - ariaProps = { - 'aria-label': __( 'Custom color picker.' ), - }; - } - const metaProps = asButtons ? { asButtons } : { loop }; + if ( ariaLabel ) { + metaProps = { ..._metaProps, 'aria-label': ariaLabel }; + } else if ( ariaLabelledby ) { + metaProps = { + ..._metaProps, + 'aria-labelledby': ariaLabelledby, + }; + } else { + metaProps = { + ..._metaProps, + 'aria-label': __( 'Custom color picker.' ), + }; + } + } return ( @@ -322,7 +331,6 @@ function UnforwardedColorPalette( /> ) } ) { ); - let ariaProps: - | { 'aria-label': string } - | { 'aria-labelledby': string } - | {} = {}; + let metaProps: + | { asButtons: false; loop?: boolean; 'aria-label': string } + | { asButtons: false; loop?: boolean; 'aria-labelledby': string } + | { asButtons: true }; - if ( ariaLabel ) { - ariaProps = { 'aria-label': ariaLabel }; - } else if ( ariaLabelledby ) { - ariaProps = { - 'aria-labelledby': ariaLabelledby, + if ( asButtons ) { + metaProps = { asButtons: true }; + } else { + const _metaProps: { asButtons: false; loop?: boolean } = { + asButtons: false, + loop, }; - } else if ( ! asButtons ) { - ariaProps = { - 'aria-label': __( 'Custom gradient picker.' ), - }; - } - const metaProps = asButtons ? { asButtons } : { loop }; + if ( ariaLabel ) { + metaProps = { ..._metaProps, 'aria-label': ariaLabel }; + } else if ( ariaLabelledby ) { + metaProps = { + ..._metaProps, + 'aria-labelledby': ariaLabelledby, + }; + } else { + metaProps = { + ..._metaProps, + 'aria-label': __( 'Custom color picker.' ), + }; + } + } return ( Date: Thu, 14 Sep 2023 00:06:54 +0100 Subject: [PATCH 16/25] Updating `color-palette` snapshot --- .../color-palette/test/__snapshots__/control.js.snap | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap b/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap index 3fa7e6fd690e8e..6b4b305f60f519 100644 --- a/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap +++ b/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap @@ -201,7 +201,7 @@ exports[`ColorPaletteControl matches the snapshot 1`] = `
Date: Thu, 14 Sep 2023 00:46:47 +0100 Subject: [PATCH 17/25] Wrapping Option implementations with `forwardRef` --- .../circular-option-picker-option.tsx | 38 ++++++++++++------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx index 62e584bcfb67ff..312a2b54885e60 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx @@ -2,12 +2,13 @@ * External dependencies */ import classnames from 'classnames'; +import type { ForwardedRef } from 'react'; /** * WordPress dependencies */ import { useInstanceId } from '@wordpress/compose'; -import { useContext, useEffect } from '@wordpress/element'; +import { forwardRef, useContext, useEffect } from '@wordpress/element'; import { Icon, check } from '@wordpress/icons'; /** @@ -25,20 +26,28 @@ import type { const hasSelectedOption = new Map(); -function OptionAsButton( props: { - id?: string; - className?: string; - isPressed?: boolean; -} ) { - return ; +function UnforwardedOptionAsButton( + props: { + id?: string; + className?: string; + isPressed?: boolean; + }, + forwardedRef: ForwardedRef< any > +) { + return ; } -function OptionAsOption( props: { - id: string; - className?: string; - isSelected?: boolean; - context: CircularOptionPickerContextProps; -} ) { +const OptionAsButton = forwardRef( UnforwardedOptionAsButton ); + +function UnforwardedOptionAsOption( + props: { + id: string; + className?: string; + isSelected?: boolean; + context: CircularOptionPickerContextProps; + }, + forwardedRef: ForwardedRef< any > +) { const { id, className, isSelected, context, ...additionalProps } = props; const { isComposite, ..._compositeState } = context; const compositeState = @@ -69,10 +78,13 @@ function OptionAsOption( props: { } ) } role="option" aria-selected={ !! isSelected } + ref={ forwardedRef } /> ); } +const OptionAsOption = forwardRef( UnforwardedOptionAsOption ); + export function Option( { className, isSelected, From 87cafaf07dadc110588a37f8c3449a6033974b18 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Thu, 14 Sep 2023 09:27:30 +0100 Subject: [PATCH 18/25] Overriding `WithLoopingDisabled` code sample to show `loop` --- .../circular-option-picker/stories/index.story.tsx | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/components/src/circular-option-picker/stories/index.story.tsx b/packages/components/src/circular-option-picker/stories/index.story.tsx index 20c3f543bb751c..45643e9d6dd6e1 100644 --- a/packages/components/src/circular-option-picker/stories/index.story.tsx +++ b/packages/components/src/circular-option-picker/stories/index.story.tsx @@ -127,6 +127,17 @@ WithLoopingDisabled.args = { ...Default.args, loop: false, }; +WithLoopingDisabled.parameters = { + docs: { + source: { + code: `} +/>`, + }, + }, +}; export const WithButtonAction = Template.bind( {} ); WithButtonAction.args = { From 0e4eb2a57b93210f72973950ca059e2bbc718d6f Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Fri, 15 Sep 2023 10:57:28 +0100 Subject: [PATCH 19/25] Refactoring `asButtons` typing --- .../src/circular-option-picker/types.ts | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/packages/components/src/circular-option-picker/types.ts b/packages/components/src/circular-option-picker/types.ts index 2c2b92ba097e54..463e519f57f303 100644 --- a/packages/components/src/circular-option-picker/types.ts +++ b/packages/components/src/circular-option-picker/types.ts @@ -41,13 +41,6 @@ type CommonCircularOptionPickerProps = { * The child elements. */ children?: ReactNode; - /** - * Whether the control should present as a set of buttons, - * each with its own tab stop. - * - * @default false - */ - asButtons?: boolean; }; type WithBaseId = { @@ -55,6 +48,13 @@ type WithBaseId = { }; type FullListboxCircularOptionPickerProps = CommonCircularOptionPickerProps & { + /** + * Whether the control should present as a set of buttons, + * each with its own tab stop. + * + * @default false + */ + asButtons?: false; /** * Prevents keyboard interaction from wrapping around. * Only used when `asButtons` is not true. @@ -79,7 +79,15 @@ export type ListboxCircularOptionPickerProps = WithBaseId & 'asButtons' | 'actions' | 'options' >; -type FullButtonsCircularOptionPickerProps = CommonCircularOptionPickerProps; +type FullButtonsCircularOptionPickerProps = CommonCircularOptionPickerProps & { + /** + * Whether the control should present as a set of buttons, + * each with its own tab stop. + * + * @default false + */ + asButtons: true; +}; export type ButtonsCircularOptionPickerProps = WithBaseId & Omit< @@ -88,8 +96,8 @@ export type ButtonsCircularOptionPickerProps = WithBaseId & >; export type CircularOptionPickerProps = - | ( { asButtons?: false } & FullListboxCircularOptionPickerProps ) - | ( { asButtons: true } & FullButtonsCircularOptionPickerProps ); + | FullListboxCircularOptionPickerProps + | FullButtonsCircularOptionPickerProps; export type DropdownLinkActionProps = { buttonProps?: Omit< From 880a1a1638e318c52ef17a81328342089b699338 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Fri, 15 Sep 2023 10:59:32 +0100 Subject: [PATCH 20/25] Refactoring `compositeState` accessors --- .../circular-option-picker.tsx | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker.tsx b/packages/components/src/circular-option-picker/circular-option-picker.tsx index b46c83ec47900c..d17c6103916852 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker.tsx @@ -80,28 +80,23 @@ function ListboxCircularOptionPicker( const rtl = isRTL(); const compositeState = useCompositeState( { baseId, loop, rtl } ); + const { setBaseId, setLoop, setRTL } = compositeState; // These are necessary as `useCompositeState` is sealed after // the first render, so although unlikely to happen, if a state // property should change, we need to process it accordingly. useEffect( () => { - compositeState.setBaseId( baseId ); - // Disabling exhaustive-deps check because it expects - // `compositeState` to be present, but doing so causes - // an infinite loop. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ baseId ] ); + setBaseId( baseId ); + }, [ setBaseId, baseId ] ); useEffect( () => { - compositeState.setLoop( !! loop ); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ loop ] ); + setLoop( loop ); + }, [ setLoop, loop ] ); useEffect( () => { - compositeState.setRTL( rtl ); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ rtl ] ); + setRTL( rtl ); + }, [ setRTL, rtl ] ); const compositeContext = { isComposite: true, From 3deef13cd409d5b94836bbd4ad9c63dcccaaf043 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Fri, 15 Sep 2023 13:10:52 +0100 Subject: [PATCH 21/25] Updating various README docs with new props --- .../src/circular-option-picker/README.md | 14 ++++++++++++++ packages/components/src/color-palette/README.md | 14 ++++++++++++++ packages/components/src/duotone-picker/README.md | 14 ++++++++++++++ packages/components/src/gradient-picker/README.md | 14 ++++++++++++++ 4 files changed, 56 insertions(+) diff --git a/packages/components/src/circular-option-picker/README.md b/packages/components/src/circular-option-picker/README.md index 122eca4fa56327..a7595daa349cd7 100644 --- a/packages/components/src/circular-option-picker/README.md +++ b/packages/components/src/circular-option-picker/README.md @@ -80,6 +80,20 @@ The child elements. - Required: No +### `asButtons`: `boolean` + +Whether the control should present as a set of buttons, each with its own tab stop. + +- Required: No +- Default: `false` + +### `loop`: `boolean` + +Prevents keyboard interaction from wrapping around. Only used when `asButtons` is not true. + +- Required: No +- Default: `true` + ## Subcomponents ### `CircularOptionPicker.ButtonAction` diff --git a/packages/components/src/color-palette/README.md b/packages/components/src/color-palette/README.md index 5db11051655b66..7b13b3905dd4ae 100644 --- a/packages/components/src/color-palette/README.md +++ b/packages/components/src/color-palette/README.md @@ -84,3 +84,17 @@ Currently active value. Callback called when a color is selected. - Required: Yes + +### `asButtons`: `boolean` + +Whether the control should present as a set of buttons, each with its own tab stop. + +- Required: No +- Default: `false` + +### `loop`: `boolean` + +Prevents keyboard interaction from wrapping around. Only used when `asButtons` is not true. + +- Required: No +- Default: `true` diff --git a/packages/components/src/duotone-picker/README.md b/packages/components/src/duotone-picker/README.md index 7b272e38dd8412..4c1b5c13c4be10 100644 --- a/packages/components/src/duotone-picker/README.md +++ b/packages/components/src/duotone-picker/README.md @@ -64,6 +64,20 @@ An array of colors for the duotone effect. Callback which is called when the duotone colors change. +### `asButtons`: `boolean` + +Whether the control should present as a set of buttons, each with its own tab stop. + +- Required: No +- Default: `false` + +### `loop`: `boolean` + +Prevents keyboard interaction from wrapping around. Only used when `asButtons` is not true. + +- Required: No +- Default: `true` + ## DuotoneSwatch Props ### `values` diff --git a/packages/components/src/gradient-picker/README.md b/packages/components/src/gradient-picker/README.md index 0901d14043eab8..6d7a482df82820 100644 --- a/packages/components/src/gradient-picker/README.md +++ b/packages/components/src/gradient-picker/README.md @@ -102,3 +102,17 @@ The heading level. Only applies in cases where gradients are provided from multi - Required: No - Default: `2` + +### `asButtons`: `boolean` + +Whether the control should present as a set of buttons, each with its own tab stop. + +- Required: No +- Default: `false` + +### `loop`: `boolean` + +Prevents keyboard interaction from wrapping around. Only used when `asButtons` is not true. + +- Required: No +- Default: `true` From 6fd9ba61bf42bf0fe514deee034f438d96ba9fd9 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Fri, 15 Sep 2023 13:23:54 +0100 Subject: [PATCH 22/25] Adding note to explain class additions --- .../circular-option-picker-option.tsx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx index 312a2b54885e60..0cfbea06538aab 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker-option.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker-option.tsx @@ -73,6 +73,12 @@ function UnforwardedOptionAsOption( { ...compositeState } as={ Button } id={ id } + // Ideally we'd let the underlying `Button` component + // handle this by passing `isPressed` as a prop. + // Unfortunately doing so also sets `aria-pressed` as + // an attribute on the element, which is incompatible + // with `role="option"`, and there is no way at this + // point to override that behaviour. className={ classnames( className, { 'is-pressed': isSelected, } ) } From 2f55f86cdce1924c0a25cd00733e04351e09c8df Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Fri, 15 Sep 2023 13:51:49 +0100 Subject: [PATCH 23/25] Refactoring `listbox` implementation to keep `children` out of option list --- .../circular-option-picker.tsx | 55 +++++++++++++------ .../src/circular-option-picker/types.ts | 10 +--- 2 files changed, 40 insertions(+), 25 deletions(-) diff --git a/packages/components/src/circular-option-picker/circular-option-picker.tsx b/packages/components/src/circular-option-picker/circular-option-picker.tsx index d17c6103916852..4a0553e22a187a 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker.tsx @@ -76,7 +76,16 @@ import { function ListboxCircularOptionPicker( props: ListboxCircularOptionPickerProps ) { - const { baseId, loop = true, children, ...additionalProps } = props; + const { + actions, + options, + baseId, + loop = true, + children, + 'aria-label': ariaLabel, + 'aria-labelledby': ariaLabelledby, + ...additionalProps + } = props; const rtl = isRTL(); const compositeState = useCompositeState( { baseId, loop, rtl } ); @@ -104,13 +113,17 @@ function ListboxCircularOptionPicker( }; return ( - + +
+ { options } +
{ children } + { actions } ); @@ -119,14 +132,16 @@ function ListboxCircularOptionPicker( function ButtonsCircularOptionPicker( props: ButtonsCircularOptionPickerProps ) { - const { children, baseId, ...additionalProps } = props; + const { actions, options, children, baseId, ...additionalProps } = props; return (
+ { options } { children } + { actions }
); @@ -135,8 +150,8 @@ function ButtonsCircularOptionPicker( function CircularOptionPicker( props: CircularOptionPickerProps ) { const { asButtons, - actions, - options, + actions: actionsProp, + options: optionsProp, children, className, ...additionalProps @@ -152,6 +167,18 @@ function CircularOptionPicker( props: CircularOptionPickerProps ) { ? ButtonsCircularOptionPicker : ListboxCircularOptionPicker; + const actions = actionsProp ? ( +
+ { actionsProp } +
+ ) : undefined; + + const options = ( +
+ { optionsProp } +
+ ); + return ( -
- { options } -
{ children } - { actions && ( -
- { actions } -
- ) } ); } diff --git a/packages/components/src/circular-option-picker/types.ts b/packages/components/src/circular-option-picker/types.ts index 463e519f57f303..f3449769c5e3fe 100644 --- a/packages/components/src/circular-option-picker/types.ts +++ b/packages/components/src/circular-option-picker/types.ts @@ -74,10 +74,7 @@ type FullListboxCircularOptionPickerProps = CommonCircularOptionPickerProps & { ); export type ListboxCircularOptionPickerProps = WithBaseId & - Omit< - FullListboxCircularOptionPickerProps, - 'asButtons' | 'actions' | 'options' - >; + Omit< FullListboxCircularOptionPickerProps, 'asButtons' >; type FullButtonsCircularOptionPickerProps = CommonCircularOptionPickerProps & { /** @@ -90,10 +87,7 @@ type FullButtonsCircularOptionPickerProps = CommonCircularOptionPickerProps & { }; export type ButtonsCircularOptionPickerProps = WithBaseId & - Omit< - FullButtonsCircularOptionPickerProps, - 'asButtons' | 'actions' | 'options' - >; + Omit< FullButtonsCircularOptionPickerProps, 'asButtons' >; export type CircularOptionPickerProps = | FullListboxCircularOptionPickerProps From a7b82e8366caa8324e71d9f39966677566364bf2 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Fri, 15 Sep 2023 14:20:15 +0100 Subject: [PATCH 24/25] Re-refactoring `listbox` implementation --- .../test/__snapshots__/control.js.snap | 57 ++++++++++--------- .../circular-option-picker.tsx | 17 +++--- 2 files changed, 38 insertions(+), 36 deletions(-) diff --git a/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap b/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap index 6b4b305f60f519..94dc53f7da1877 100644 --- a/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap +++ b/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap @@ -199,43 +199,46 @@ exports[`ColorPaletteControl matches the snapshot 1`] = `
-
diff --git a/packages/components/src/circular-option-picker/circular-option-picker.tsx b/packages/components/src/circular-option-picker/circular-option-picker.tsx index 4a0553e22a187a..08c5ad94a34cb7 100644 --- a/packages/components/src/circular-option-picker/circular-option-picker.tsx +++ b/packages/components/src/circular-option-picker/circular-option-picker.tsx @@ -80,10 +80,9 @@ function ListboxCircularOptionPicker( actions, options, baseId, + className, loop = true, children, - 'aria-label': ariaLabel, - 'aria-labelledby': ariaLabelledby, ...additionalProps } = props; const rtl = isRTL(); @@ -113,19 +112,19 @@ function ListboxCircularOptionPicker( }; return ( - +
-
{ options } -
+ { children } { actions } - +
); } From 87a3b2ba9ea0770718cc9d5dd579a85fd4891025 Mon Sep 17 00:00:00 2001 From: Andrew Hayward Date: Fri, 15 Sep 2023 14:27:18 +0100 Subject: [PATCH 25/25] Removing redundant `@default` --- packages/components/src/circular-option-picker/types.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/components/src/circular-option-picker/types.ts b/packages/components/src/circular-option-picker/types.ts index f3449769c5e3fe..5a54a334124351 100644 --- a/packages/components/src/circular-option-picker/types.ts +++ b/packages/components/src/circular-option-picker/types.ts @@ -51,8 +51,6 @@ type FullListboxCircularOptionPickerProps = CommonCircularOptionPickerProps & { /** * Whether the control should present as a set of buttons, * each with its own tab stop. - * - * @default false */ asButtons?: false; /**