Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making Circular Option Picker a listbox #52255

Merged
merged 29 commits into from
Sep 4, 2023
Merged

Making Circular Option Picker a listbox #52255

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
9232f14
Making Circular Option Picker a `listbox`
Jul 3, 2023
1f87fc5
Merge branch 'WordPress:trunk' into 35292/improve_color_palette_keybo…
andrewhayward Aug 1, 2023
309e523
Various updates
Aug 15, 2023
84ac262
Merge branch 'trunk' into 35292/improve_color_palette_keyboard_access…
Aug 15, 2023
37016b3
Merge branch 'trunk' into 35290/improve_color_palette_keyboard_access…
Aug 21, 2023
72e8889
Fixing tests for `core/cover`
Aug 21, 2023
d932389
Fixing tests for `BorderControl`
Aug 21, 2023
b331d7c
Fixing tests for `BorderControl`
Aug 23, 2023
a90e82f
Adding default values for ARIA labels
Aug 29, 2023
142b7c2
Fixing merge conflict
Aug 29, 2023
7336b69
Removing redundant `ColorPalette` test
Aug 29, 2023
3692ccd
Updating custom color tests
Aug 29, 2023
fc000ee
Removing redundant snapshots
Aug 29, 2023
17f8837
Updating broken snapshot
Aug 29, 2023
a61af94
Merge branch 'trunk' into 35292/improve_color_palette_keyboard_access…
Aug 29, 2023
5385c6e
Updating CHANGELOG.md
Aug 29, 2023
eb6658a
Fixing various E2E tests
Aug 29, 2023
718f81f
Updating test for new keyboard navigation
Aug 29, 2023
d20d985
Updating E2E tests
Aug 29, 2023
432af69
Fixing navigation-colors E2E test
Aug 30, 2023
65a74fb
Fixing E2E tests for cover and heading blocks
Aug 30, 2023
01693bb
Removing `option` constant
Aug 30, 2023
aaa8c45
Removing redundant type coercion
Sep 1, 2023
149c4da
Enforcing mutual exclusivity of `-label`/`-labelledby`
Sep 1, 2023
8c22df2
Making label exclusivity clearer
Sep 1, 2023
78d88d0
Merge branch 'trunk' into 35292/improve_color_palette_keyboard_access…
Sep 1, 2023
e808582
Updating CHANGELOG.md
Sep 1, 2023
b15497f
Fixing broken test
Sep 1, 2023
717469b
Merge branch 'trunk' into 35292/improve_color_palette_keyboard_access…
ciampo Sep 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 69 additions & 17 deletions packages/components/src/circular-option-picker/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,35 +7,50 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { Icon, check } from '@wordpress/icons';
import { createContext, useContext } from '@wordpress/element';

/**
* Internal dependencies
*/
import Button from '../button';
import {
Composite,
CompositeGroup,
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( {} );

export function Option( {
className,
isSelected,
selectedIconProps,
tooltipText,
...additionalProps
}: OptionProps ) {
const optionButton = (
<Button
const compositeState = useContext( CircularOptionPickerContext );

const optionControl = (
<CompositeItem
as={ Button }
className={ 'components-circular-option-picker__option' }
isPressed={ isSelected }
andrewhayward marked this conversation as resolved.
Show resolved Hide resolved
className="components-circular-option-picker__option"
{ ...additionalProps }
{ ...compositeState }
/>
);

return (
<div
className={ classnames(
Expand All @@ -44,9 +59,9 @@ export function Option( {
) }
>
{ tooltipText ? (
<Tooltip text={ tooltipText }>{ optionButton }</Tooltip>
<Tooltip text={ tooltipText }>{ optionControl }</Tooltip>
) : (
optionButton
optionControl
) }
{ isSelected && (
<Icon
Expand All @@ -58,6 +73,34 @@ export function Option( {
);
}

export function OptionGroup( {
className,
options,
...additionalProps
}: OptionGroupProps ) {
const compositeState = useContext( CircularOptionPickerContext );

// This is unlikely to happen, but on the off-chance that we've ended up
// with a list of `OptionGroup`s, we will just return those instead.
if ( Array.isArray( options ) && options[ 0 ] instanceof OptionGroup ) {
return <>options</>;
andrewhayward marked this conversation as resolved.
Show resolved Hide resolved
}

return (
<CompositeGroup
role={ 'group' }
{ ...additionalProps }
{ ...compositeState }
className={ classnames(
'components-circular-option-picker__swatches',
className
) }
>
{ options }
</CompositeGroup>
);
}

export function DropdownLinkAction( {
buttonProps,
className,
Expand Down Expand Up @@ -152,28 +195,37 @@ export function ButtonAction( {
*/

function CircularOptionPicker( props: CircularOptionPickerProps ) {
const { actions, className, options, children } = props;
const { actions, className, options, children, loop = true } = props;
const compositeState = useCompositeState( { loop } );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not up to date on this, but is there a reason why we're using useCompositeState instead of useCompositeStore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're still using reakit under the hood for this, which is where useCompositeState is coming from. As and when we move composite to ariakit we'll have to change that.


return (
<div
<Composite
{ ...compositeState }
role={ 'listbox' }
className={ classnames(
'components-circular-option-picker',
className
) }
>
<div className="components-circular-option-picker__swatches">
{ options }
</div>
{ children }
{ actions && (
<div className="components-circular-option-picker__custom-clear-wrapper">
{ actions }
</div>
) }
</div>
<CircularOptionPickerContext.Provider value={ compositeState }>
{ Array.isArray( options ) ? (
andrewhayward marked this conversation as resolved.
Show resolved Hide resolved
<OptionGroup options={ options } />
) : (
options
) }
{ children }
{ actions && (
<div className="components-circular-option-picker__custom-clear-wrapper">
{ actions }
</div>
) }
</CircularOptionPickerContext.Provider>
</Composite>
);
}

CircularOptionPicker.Option = Option;
CircularOptionPicker.OptionGroup = OptionGroup;
CircularOptionPicker.ButtonAction = ButtonAction;
CircularOptionPicker.DropdownLinkAction = DropdownLinkAction;

Expand Down
10 changes: 10 additions & 0 deletions packages/components/src/circular-option-picker/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ export type CircularOptionPickerProps = {
* The child elements.
*/
children?: ReactNode;
/**
* Whether the keyboard interaction should wrap around.
* Defaults to `true`.
andrewhayward marked this conversation as resolved.
Show resolved Hide resolved
*/
loop?: boolean;
};

export type DropdownLinkActionProps = {
Expand All @@ -48,6 +53,11 @@ export type DropdownLinkActionProps = {
className?: string;
};

export type OptionGroupProps = {
className?: string;
options: ReactNode;
};

export type OptionProps = Omit<
WordPressComponentProps< ButtonAsButtonProps, 'button', false >,
'isPressed' | 'className'
Expand Down
60 changes: 34 additions & 26 deletions packages/components/src/color-palette/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { useInstanceId } from '@wordpress/compose';
import { __, sprintf } from '@wordpress/i18n';
import { useCallback, useMemo, useState, forwardRef } from '@wordpress/element';

Expand Down Expand Up @@ -47,7 +48,7 @@ function SinglePalette( {
colors,
onChange,
value,
actions,
...otherProps
}: SinglePaletteProps ) {
const colorOptions = useMemo( () => {
return colors.map( ( { color, name }, index ) => {
Expand Down Expand Up @@ -91,10 +92,10 @@ function SinglePalette( {
}, [ colors, value, onChange, clearColor ] );

return (
<CircularOptionPicker
<CircularOptionPicker.OptionGroup
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure about modeling the multiple palette case as groups in a single listbox. From reading the WAI guidance, I think a listbox is assumed to be one-dimensional (i.e. a list not a grid), with a aria-orientation to dictate which arrow keys to use.

What do you think about dropping the group construct and modeling them as separate listboxes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about dropping the group construct and modeling them as separate listboxes?

After interacting with the ColorPalette storybook demo with multiple origins, I also got the feeling that having each OptionGroup as a separate tab stop feels better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further discussion, we've decided that we're going to keep the current behaviour, and iterate if/when necessary in a future PR.

className={ className }
options={ colorOptions }
actions={ actions }
{ ...otherProps }
/>
);
}
Expand All @@ -105,19 +106,21 @@ function MultiplePalettes( {
colors,
onChange,
value,
actions,
ciampo marked this conversation as resolved.
Show resolved Hide resolved
headingLevel,
}: MultiplePalettesProps ) {
const instanceId = useInstanceId( MultiplePalettes );

if ( colors.length === 0 ) {
return null;
}

return (
<VStack spacing={ 3 } className={ className }>
{ colors.map( ( { name, colors: colorPalette }, index ) => {
const id = `color-palette-${ instanceId }-${ index }`;
andrewhayward marked this conversation as resolved.
Show resolved Hide resolved
return (
<VStack spacing={ 2 } key={ index }>
<ColorHeading level={ headingLevel }>
<ColorHeading id={ id } level={ headingLevel }>
{ name }
</ColorHeading>
<SinglePalette
Expand All @@ -127,9 +130,7 @@ function MultiplePalettes( {
onChange( newColor, index )
}
value={ value }
actions={
colors.length === index + 1 ? actions : null
}
aria-labelledby={ id }
/>
</VStack>
);
Expand Down Expand Up @@ -234,18 +235,17 @@ function UnforwardedColorPalette(
: __( 'Custom color picker.' );

const paletteCommonProps = {
clearable,
clearColor,
onChange,
value,
actions: !! clearable && (
<CircularOptionPicker.ButtonAction onClick={ clearColor }>
{ __( 'Clear' ) }
</CircularOptionPicker.ButtonAction>
),
headingLevel,
};

const actions = !! clearable && (
<CircularOptionPicker.ButtonAction onClick={ clearColor }>
{ __( 'Clear' ) }
</CircularOptionPicker.ButtonAction>
);

return (
<VStack spacing={ 3 } ref={ forwardedRef } { ...otherProps }>
{ ! disableCustomColors && (
Expand Down Expand Up @@ -298,17 +298,25 @@ function UnforwardedColorPalette(
) }
/>
) }
{ hasMultipleColorOrigins ? (
<MultiplePalettes
{ ...paletteCommonProps }
colors={ colors as PaletteObject[] }
/>
) : (
<SinglePalette
{ ...paletteCommonProps }
colors={ colors as ColorObject[] }
/>
) }
<CircularOptionPicker
actions={ actions }
options={
hasMultipleColorOrigins ? (
<MultiplePalettes
{ ...paletteCommonProps }
headingLevel={ headingLevel }
colors={ colors as PaletteObject[] }
value={ value }
/>
) : (
<SinglePalette
{ ...paletteCommonProps }
colors={ colors as ColorObject[] }
value={ value }
/>
)
}
/>
</VStack>
);
}
Expand Down
4 changes: 3 additions & 1 deletion packages/components/src/color-palette/stories/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,10 @@ export default meta;

const Template: ComponentStory< typeof ColorPalette > = ( {
onChange,
value,
...args
} ) => {
const [ color, setColor ] = useState< string | undefined >();
const [ color, setColor ] = useState< string | undefined >( value );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Marking this as TODO so we don't forget to address it)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, what exactly was the TODO item related to this line of code?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewhayward would you happen to know what needed to be done around this line?


return (
<SlotFillProvider>
Expand All @@ -60,6 +61,7 @@ Default.args = {
{ name: 'White', color: '#fff' },
{ name: 'Blue', color: '#00f' },
],
value: '#00f',
};

export const MultipleOrigins = Template.bind( {} );
Expand Down
Loading