Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 12 additions & 1 deletion src/components/input/SelectContext.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,21 @@
import { createContext } from 'preact';

export type SelectContextType<T = unknown> = {
type SingleSelectContext<T> = {
selectValue: (newValue: T) => void;
value: T;
multiple: false;
};

type MultiSelectContext<T> = {
selectValue: (newValue: T[]) => void;
value: T[];
multiple: true;
};

export type SelectContextType<T = unknown> =
| SingleSelectContext<T>
| MultiSelectContext<T>;

const SelectContext = createContext<SelectContextType | null>(null);

export default SelectContext;
143 changes: 104 additions & 39 deletions src/components/input/SelectNext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@ export type SelectOptionStatus = {
};

export type SelectOptionProps<T> = {
value: T;
/**
* An undefined value in a multiple select will cause the selection to reset
* to an empty array.
*/
value: T | undefined;

disabled?: boolean;
children:
| ComponentChildren
Expand Down Expand Up @@ -59,8 +64,34 @@ function SelectOption<T>({
throw new Error('Select.Option can only be used as Select child');
}

const { selectValue, value: currentValue } = selectContext;
const selected = !disabled && currentValue === value;
const { selectValue, value: currentValue, multiple } = selectContext;
const selected =
!disabled &&
((multiple && currentValue.includes(value)) || currentValue === value);

const selectOrToggle = useCallback(() => {
// In single-select, just set current value
if (!multiple) {
selectValue(value);
return;
}

// In multi-select, clear selection for nullish values
if (!value) {
selectValue([]);
return;
}
Comment on lines +80 to +83
Copy link
Contributor Author

@acelaya acelaya Jul 2, 2024

Choose a reason for hiding this comment

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

This is a bit opinionated and not entirely obvious behavior.

Perhaps a better option would be to allow <SelectOption /> to provide its own onChange onSelect, in which case it gets invoked instead of the default selectValue(...) logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggested above that it might help to avoid errors if the component just threw an error if in multi-select mode and a non-array value is passed.


// In multi-select, toggle clicked items
const index = currentValue.indexOf(value);
if (index === -1) {
selectValue([...currentValue, value]);
} else {
const copy = [...currentValue];
copy.splice(index, 1);
selectValue(copy);
}
}, [currentValue, multiple, selectValue, value]);

return (
<li
Expand All @@ -75,13 +106,13 @@ function SelectOption<T>({
)}
onClick={() => {
if (!disabled) {
selectValue(value);
selectOrToggle();
}
}}
onKeyPress={e => {
if (!disabled && ['Enter', 'Space'].includes(e.code)) {
e.preventDefault();
selectValue(value);
selectOrToggle();
}
}}
role="option"
Expand Down Expand Up @@ -215,42 +246,59 @@ function useListboxPositioning(
}, [adjustListboxPositioning, asPopover]);
}

export type SelectProps<T> = CompositeProps & {
type SingleValueProps<T> = {
value: T;
onChange: (newValue: T) => void;
buttonContent?: ComponentChildren;
disabled?: boolean;
};

/**
* `id` attribute for the toggle button. This is useful to associate a label
* with the control.
*/
buttonId?: string;
type MultiValueProps<T> = {
value: T[];
onChange: (newValue: T[]) => void;
};

/** Additional classes to pass to container */
containerClasses?: string | string[];
/** Additional classes to pass to toggle button */
buttonClasses?: string | string[];
/** Additional classes to pass to listbox */
listboxClasses?: string | string[];
export type SelectProps<T> = CompositeProps &
(SingleValueProps<T> | MultiValueProps<T>) & {
buttonContent?: ComponentChildren;
disabled?: boolean;

/**
* Align the listbox to the right.
* Useful when the listbox is bigger than the toggle button and this component
* is rendered next to the right side of the page/container.
* Defaults to false.
*/
right?: boolean;
/**
* Whether this select should allow multi-selection or not.
* When this is true, the listbox is kept open when an option is selected
* and the value must be an array.
* Defaults to false.
*/
multiple?: boolean;

'aria-label'?: string;
'aria-labelledby'?: string;
/**
* `id` attribute for the toggle button. This is useful to associate a label
* with the control.
*/
buttonId?: string;

/**
* Used to determine if the listbox should use the popover API.
* Defaults to true, as long as the browser supports it.
*/
listboxAsPopover?: boolean;
};
/** Additional classes to pass to container */
containerClasses?: string | string[];
/** Additional classes to pass to toggle button */
buttonClasses?: string | string[];
/** Additional classes to pass to listbox */
listboxClasses?: string | string[];

/**
* Align the listbox to the right.
* Useful when the listbox is bigger than the toggle button and this component
* is rendered next to the right side of the page/container.
* Defaults to false.
*/
right?: boolean;

'aria-label'?: string;
'aria-labelledby'?: string;

/**
* Used to determine if the listbox should use the popover API.
* Defaults to true, as long as the browser supports it.
*/
listboxAsPopover?: boolean;
};

function SelectMain<T>({
buttonContent,
Expand All @@ -264,11 +312,16 @@ function SelectMain<T>({
listboxClasses,
containerClasses,
right = false,
multiple = false,
'aria-label': ariaLabel,
'aria-labelledby': ariaLabelledBy,
/* eslint-disable-next-line no-prototype-builtins */
listboxAsPopover = HTMLElement.prototype.hasOwnProperty('popover'),
}: SelectProps<T>) {
if (multiple && !Array.isArray(value)) {
throw new Error('When `multiple` is true, the value must be an array');
}

const wrapperRef = useRef<HTMLDivElement | null>(null);
const listboxRef = useRef<HTMLUListElement | null>(null);
const [listboxOpen, setListboxOpen] = useState(false);
Expand All @@ -295,11 +348,14 @@ function SelectMain<T>({
);

const selectValue = useCallback(
(newValue: unknown) => {
onChange(newValue as T);
closeListbox();
(value: unknown) => {
onChange(value as any);
// In multi-select mode, keep list open when selecting values
if (!multiple) {
closeListbox();
}
},
[closeListbox, onChange],
[onChange, multiple, closeListbox],
);

// When clicking away, focusing away or pressing `Esc`, close the listbox
Expand Down Expand Up @@ -369,7 +425,15 @@ function SelectMain<T>({
{listboxOpen ? <MenuCollapseIcon /> : <MenuExpandIcon />}
</div>
</button>
<SelectContext.Provider value={{ selectValue, value }}>

<SelectContext.Provider
value={{
// Explicit type casting needed here
value: value as typeof multiple extends false ? T : T[],
selectValue,
multiple,
}}
>
<ul
className={classnames(
'absolute z-5 max-h-80 overflow-y-auto',
Expand All @@ -387,6 +451,7 @@ function SelectMain<T>({
role="listbox"
ref={listboxRef}
id={listboxId}
aria-multiselectable={multiple}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

aria-labelledby={buttonId ?? defaultButtonId}
aria-orientation="vertical"
data-testid="select-listbox"
Expand Down
96 changes: 91 additions & 5 deletions src/components/input/test/SelectNext-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ describe('SelectNext', () => {

const wrapper = mount(
<SelectNext value={undefined} onChange={sinon.stub()} {...props}>
<SelectNext.Option value={undefined}>
<span data-testid="reset-option">Reset</span>
</SelectNext.Option>
{items.map(item => (
<SelectNext.Option
value={item}
Expand Down Expand Up @@ -87,19 +90,20 @@ describe('SelectNext', () => {
return listboxTop < buttonTop;
};

const clickOption = (wrapper, id) =>
wrapper.find(`[data-testid="option-${id}"]`).simulate('click');

it('changes selected value when an option is clicked', () => {
const onChange = sinon.stub();
const wrapper = createComponent({ onChange });
const clickOption = index =>
wrapper.find(`[data-testid="option-${index}"]`).simulate('click');

clickOption(3);
clickOption(wrapper, 3);
assert.calledWith(onChange.lastCall, items[2]);

clickOption(5);
clickOption(wrapper, 5);
assert.calledWith(onChange.lastCall, items[4]);

clickOption(1);
clickOption(wrapper, 1);
assert.calledWith(onChange.lastCall, items[0]);
});

Expand Down Expand Up @@ -385,6 +389,71 @@ describe('SelectNext', () => {
});
});

context('when multi-selection is enabled', () => {
it('throws if multiple is true and the value is not an array', async () => {
assert.throws(
() => createComponent({ multiple: true }),
'When `multiple` is true, the value must be an array',
);
});

it('keeps listbox open when an option is selected if multiple is true', async () => {
const wrapper = createComponent({ multiple: true, value: [] });

toggleListbox(wrapper);
assert.isFalse(isListboxClosed(wrapper));

clickOption(wrapper, 1);

// After clicking an option, the listbox is still open
assert.isFalse(isListboxClosed(wrapper));
});

it('allows multiple items to be selected when the value is an array', () => {
const onChange = sinon.stub();
const wrapper = createComponent({
multiple: true,
value: [items[0], items[2]],
onChange,
});

toggleListbox(wrapper);
clickOption(wrapper, 2);

// When a not-yet-selected item is clicked, it will be selected
assert.calledWith(onChange, [items[0], items[2], items[1]]);
});

it('allows deselecting already selected options', () => {
const onChange = sinon.stub();
const wrapper = createComponent({
multiple: true,
value: [items[0], items[2]],
onChange,
});

toggleListbox(wrapper);
clickOption(wrapper, 3);

// When an already selected item is clicked, it will be de-selected
assert.calledWith(onChange, [items[0]]);
});

it('resets selection when option value is nullish and select value is an array', () => {
const onChange = sinon.stub();
const wrapper = createComponent({
multiple: true,
value: [items[0], items[2]],
onChange,
});

toggleListbox(wrapper);
wrapper.find(`[data-testid="reset-option"]`).simulate('click');

assert.calledWith(onChange, []);
});
});

it(
'should pass a11y checks',
checkAccessibility([
Expand All @@ -405,6 +474,23 @@ describe('SelectNext', () => {
);
toggleListbox(wrapper);

return wrapper;
},
},
{
name: 'Open Multi-Select listbox',
content: () => {
const wrapper = createComponent(
{
buttonContent: 'Select',
'aria-label': 'Select',
value: [items[1], items[3]],
multiple: true,
},
{ optionsChildrenAsCallback: false },
);
toggleListbox(wrapper);

return wrapper;
},
},
Expand Down
Loading