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.

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

Expose Select and MultiSelect aliases for SelectNext #1619

Merged
merged 1 commit into from
Jul 17, 2024
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
114 changes: 69 additions & 45 deletions src/components/input/SelectNext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -257,52 +257,57 @@ type MultiValueProps<T> = {
onChange: (newValue: T[]) => void;
};

export type SelectProps<T> = CompositeProps &
(SingleValueProps<T> | MultiValueProps<T>) & {
buttonContent?: ComponentChildren;
disabled?: boolean;
type BaseSelectProps = CompositeProps & {
buttonContent?: ComponentChildren;
disabled?: 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;
/**
* `id` attribute for the toggle button. This is useful to associate a label
* with the control.
*/
buttonId?: string;

/**
* `id` attribute for the toggle button. This is useful to associate a label
* with the control.
*/
buttonId?: string;
/** 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[];

/** 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;

/**
* 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;

'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;

/**
* Used to determine if the listbox should use the popover API.
* Defaults to true, as long as the browser supports it.
*/
listboxAsPopover?: boolean;
/** A callback passed to the listbox onScroll */
onListboxScroll?: JSX.HTMLAttributes<HTMLUListElement>['onScroll'];
};

export type SelectProps<T> = BaseSelectProps & SingleValueProps<T>;

/** A callback passed to the listbox onScroll */
onListboxScroll?: JSX.HTMLAttributes<HTMLUListElement>['onScroll'];
};
export type MultiSelectProps<T> = BaseSelectProps & MultiValueProps<T>;

export type SelectNextProps<T> = (SelectProps<T> | MultiSelectProps<T>) & {
Copy link
Member

Choose a reason for hiding this comment

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

Other than being a breaking change, are there any reasons to keep SelectNext around? Can we replace all uses with either Select or MultiSelect?

Copy link
Contributor Author

@acelaya acelaya Jul 17, 2024

Choose a reason for hiding this comment

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

Nop, just the breaking change. I already have PRs branches in both client and LMS replacing all SelectNext instances and they all transparently work.

/**
* 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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered moving this to SingleValueProps and MultiValueProps, but that breaks other things, so I decided to leave it as is.

With Select and MultiSelect, this prop's value will be implicit and consumers will not have to provide it, so it's not too bad.

};

function SelectMain<T>({
buttonContent,
Expand All @@ -322,7 +327,7 @@ function SelectMain<T>({
'aria-labelledby': ariaLabelledBy,
/* eslint-disable-next-line no-prototype-builtins */
listboxAsPopover = HTMLElement.prototype.hasOwnProperty('popover'),
}: SelectProps<T>) {
}: SelectNextProps<T>) {
if (multiple && !Array.isArray(value)) {
throw new Error('When `multiple` is true, the value must be an array');
}
Expand Down Expand Up @@ -474,8 +479,27 @@ function SelectMain<T>({
);
}

SelectMain.displayName = 'SelectNext';

const SelectNext = Object.assign(SelectMain, { Option: SelectOption });

export default SelectNext;
export const SelectNext = Object.assign(SelectMain, {
Option: SelectOption,
displayName: 'SelectNext',
});

export const Select = Object.assign(
function <T>(props: SelectProps<T>) {
// Calling the function directly instead of returning a JSX element, to
// avoid an unnecessary extra layer in the component tree
// eslint-disable-next-line new-cap
return SelectNext({ ...props, multiple: false });
},
{ Option: SelectOption, displayName: 'Select' },
);

export const MultiSelect = Object.assign(
Copy link
Contributor Author

@acelaya acelaya Jul 16, 2024

Choose a reason for hiding this comment

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

I chose MultiSelect over SelectMulti for consistency with SingleValueProps and MultiValueProps, but the latter allows for a nicer grouping of symbols when sorted alphabetically.

function <T>(props: MultiSelectProps<T>) {
// Calling the function directly instead of returning a JSX element, to
// avoid an unnecessary extra layer in the component tree
// eslint-disable-next-line new-cap
return SelectNext({ ...props, multiple: true });
},
{ Option: SelectOption, displayName: 'MultiSelect' },
);
8 changes: 6 additions & 2 deletions src/components/input/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export { default as IconButton } from './IconButton';
export { default as Input } from './Input';
export { default as InputGroup } from './InputGroup';
export { default as OptionButton } from './OptionButton';
export { default as SelectNext } from './SelectNext';
export { SelectNext, Select, MultiSelect } from './SelectNext';
export { default as Textarea } from './Textarea';

export type { ButtonProps } from './Button';
Expand All @@ -15,5 +15,9 @@ export type { IconButtonProps } from './IconButton';
export type { InputProps } from './Input';
export type { InputGroupProps } from './InputGroup';
export type { OptionButtonProps } from './OptionButton';
export type { SelectProps as SelectNextProps } from './SelectNext';
export type {
MultiSelectProps,
SelectNextProps,
SelectProps,
} from './SelectNext';
export type { TextareaProps } from './Textarea';
88 changes: 60 additions & 28 deletions src/components/input/test/SelectNext-test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { checkAccessibility, waitFor } from '@hypothesis/frontend-testing';
import { mount } from 'enzyme';

import SelectNext from '../SelectNext';
import { MultiSelect, Select, SelectNext } from '../SelectNext';

describe('SelectNext', () => {
let wrappers;
Expand All @@ -21,20 +21,26 @@ describe('SelectNext', () => {
* Whether to renders SelectNext.Option children with callback notation.
* Used primarily to test and cover both branches.
* Defaults to true.
* @param {MultiSelect | Select | SelectNext} [options.Component] -
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not completely sure this JSDoc is correct

Copy link
Member

Choose a reason for hiding this comment

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

The JSDoc here is only for information and not typed checked, so this is fine as long as the meaning is clear.

* The actual "select" component to use. Defaults to `SelectNext`.
*/
const createComponent = (props = {}, options = {}) => {
const { paddingTop = 0, optionsChildrenAsCallback = true } = options;
const {
paddingTop = 0,
optionsChildrenAsCallback = true,
Component = SelectNext,
} = options;
const container = document.createElement('div');
container.style.paddingTop = `${paddingTop}px`;
document.body.append(container);

const wrapper = mount(
<SelectNext value={undefined} onChange={sinon.stub()} {...props}>
<SelectNext.Option value={undefined}>
<Component value={undefined} onChange={sinon.stub()} {...props}>
<Component.Option value={undefined}>
<span data-testid="reset-option">Reset</span>
</SelectNext.Option>
</Component.Option>
{items.map(item => (
<SelectNext.Option
<Component.Option
value={item}
disabled={item.id === '4'}
key={item.id}
Expand All @@ -50,9 +56,9 @@ describe('SelectNext', () => {
</span>
)
)}
</SelectNext.Option>
</Component.Option>
))}
</SelectNext>,
</Component>,
{ attachTo: container },
);

Expand Down Expand Up @@ -93,18 +99,29 @@ describe('SelectNext', () => {
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 });
[
{
title: 'changes selected value when an option is clicked',
},
{
title:
'changes selected value when an option is clicked, via Select alias',
options: { Component: Select },
},
].forEach(({ title, options = {} }) => {
it(title, () => {
const onChange = sinon.stub();
const wrapper = createComponent({ onChange }, options);

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

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

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

it('does not change selected value when a disabled option is clicked', () => {
Expand Down Expand Up @@ -409,19 +426,34 @@ describe('SelectNext', () => {
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,
});
[
{
title:
'allows multiple items to be selected when the value is an array',
extraProps: { multiple: true },
},
{
title: 'allows same behavior via MultiSelect alias',
options: { Component: MultiSelect },
},
].forEach(({ title, extraProps = {}, options = {} }) => {
it(title, () => {
const onChange = sinon.stub();
const wrapper = createComponent(
{
value: [items[0], items[2]],
onChange,
...extraProps,
},
options,
);

toggleListbox(wrapper);
clickOption(wrapper, 2);
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]]);
// 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', () => {
Expand Down
4 changes: 4 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ export {
IconButton,
Input,
InputGroup,
MultiSelect,
OptionButton,
Select,
SelectNext,
Textarea,
} from './components/input';
Expand Down Expand Up @@ -115,7 +117,9 @@ export type {
IconButtonProps,
InputProps,
InputGroupProps,
MultiSelectProps,
OptionButtonProps,
SelectProps,
SelectNextProps,
TextareaProps,
} from './components/input';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { useId, useState } from 'preact/hooks';

import { Link } from '../../../..';
import type { SelectNextProps } from '../../../../components/input';
import SelectNext from '../../../../components/input/SelectNext';
import { SelectNext } from '../../../../components/input/SelectNext';
import SelectNextInInputGroup from '../../../examples/select-next-in-input-group';
import SelectNextWithManyOptions from '../../../examples/select-next-with-custom-options';
import Library from '../../Library';
Expand Down Expand Up @@ -99,14 +99,15 @@ export default function SelectNextPage() {
title="SelectNext"
intro={
<p>
<code>SelectNext</code> is a composite component which behaves like
<code>SelectNext</code> (and its aliases <code>Select</code> and{' '}
<code>MultiSelect</code>) are composite components which behave like
the native <code>{'<select>'}</code> element.
</p>
}
>
<Library.Section>
<Library.Pattern>
<Library.Usage componentName="SelectNext" />
<Library.Usage componentName="MultiSelect, Select, SelectNext" />
Comment on lines +102 to +110
Copy link
Contributor Author

Choose a reason for hiding this comment

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

image


<Library.Example>
<Library.Demo
Expand Down Expand Up @@ -436,6 +437,11 @@ export default function SelectNextPage() {
be an array and <code>onChange</code> will receive an array as
an argument.
</p>
<p>
This prop cannot be provided to the <code>Select</code> and{' '}
<code>MultiSelect</code> aliases, where it is implicitly{' '}
<code>false</code> and <code>true</code> respectively.
</p>
</Library.InfoItem>
<Library.InfoItem label="type">
<code>boolean</code>
Expand All @@ -449,6 +455,11 @@ export default function SelectNextPage() {
exampleFile="select-next-multiple"
withSource
/>
<Library.Demo
title="Using MultiSelect component"
exampleFile="select-next-multi-select"
withSource
/>
</Library.Example>
<Library.Example title="onListboxScroll">
<Library.Info>
Expand Down
12 changes: 6 additions & 6 deletions src/pattern-library/examples/select-next-basic.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useId, useState } from 'preact/hooks';

import { SelectNext } from '../..';
import { Select } from '../..';

const items = [
{ id: '1', name: 'All students' },
Expand All @@ -17,18 +17,18 @@ export default function App() {
return (
<div className="w-96 mx-auto">
<label htmlFor={selectId}>Select a person</label>
<SelectNext
<Select
value={value}
onChange={setSelected}
onChange={newValue => setSelected(newValue)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used an inline callback here just to demonstrate type is properly inferred with Select, as this file is type-checked.

buttonId={selectId}
buttonContent={value ? value.name : <>Select one…</>}
>
{items.map(item => (
<SelectNext.Option value={item} key={item.id}>
<Select.Option value={item} key={item.id}>
{item.name}
</SelectNext.Option>
</Select.Option>
))}
</SelectNext>
</Select>
</div>
);
}
Loading