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

Refactor Select documentation to no longer mention SelectNext #1620

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
2 changes: 1 addition & 1 deletion docs/examples.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@ You can then reference this file from the web-based pattern library, passing the
There are some considerations and limitations when working with example files.

- They all need to have the `tsx` extension.
- Nested directories are not supported, so it's a good idea to add contextual prefixes to example files. For example, all files related with `SelectNext` can be prefixed with `select-next` to make sure they are all grouped together.
- Nested directories are not supported, so it's a good idea to add contextual prefixes to example files. For example, all files related with `Select` can be prefixed with `select` to make sure they are all grouped together.
- Example files need to have a Preact component as their default export.
- When generating the source code snippet, import statements are stripped out, to avoid internal module references which are not relevant for the library consumers.
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ function SelectOption<T>({
}: SelectOptionProps<T>) {
const selectContext = useContext(SelectContext);
if (!selectContext) {
throw new Error('Select.Option can only be used as Select child');
throw new Error(
'Select.Option can only be used as Select or MultiSelect child',
);
}

const { selectValue, value: currentValue, multiple } = selectContext;
Expand Down Expand Up @@ -134,7 +136,7 @@ function SelectOption<T>({
);
}

SelectOption.displayName = 'SelectNext.Option';
SelectOption.displayName = 'Select.Option';

/** Small space to apply between the toggle button and the listbox */
const LISTBOX_TOGGLE_GAP = '.25rem';
Expand Down Expand Up @@ -299,6 +301,9 @@ export type SelectProps<T> = BaseSelectProps & SingleValueProps<T>;

export type MultiSelectProps<T> = BaseSelectProps & MultiValueProps<T>;

/**
* @deprecated Use `Select` or `MultiSelect` components instead
*/
export type SelectNextProps<T> = (SelectProps<T> | MultiSelectProps<T>) & {
/**
* Whether this select should allow multi-selection or not.
Expand Down Expand Up @@ -479,6 +484,9 @@ function SelectMain<T>({
);
}

/**
* @deprecated Use `Select` or `MultiSelect` components instead
*/
export const SelectNext = Object.assign(SelectMain, {
Option: SelectOption,
displayName: 'SelectNext',
Expand All @@ -489,7 +497,7 @@ export const Select = Object.assign(
// 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 });
return SelectMain({ ...props, multiple: false });
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 can save one step here by directly calling SelectMain, which is going to be kept as the internal component long-term.

},
{ Option: SelectOption, displayName: 'Select' },
);
Expand All @@ -499,7 +507,7 @@ export const MultiSelect = Object.assign(
// 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 });
return SelectMain({ ...props, multiple: true });
},
{ Option: SelectOption, displayName: 'MultiSelect' },
);
8 changes: 2 additions & 6 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 { SelectNext, Select, MultiSelect } from './SelectNext';
export { SelectNext, Select, MultiSelect } from './Select';
export { default as Textarea } from './Textarea';

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

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

describe('SelectNext', () => {
describe('Select', () => {
let wrappers;
const items = [
{ id: '1', name: 'All students' },
Expand All @@ -18,17 +18,17 @@ describe('SelectNext', () => {
* @param {number} [options.paddingTop] - Extra padding top for the container.
* Defaults to 0.
* @param {boolean} [options.optionsChildrenAsCallback] -
* Whether to renders SelectNext.Option children with callback notation.
* Whether to render Select.Option children with callback notation.
* Used primarily to test and cover both branches.
* Defaults to true.
* @param {MultiSelect | Select | SelectNext} [options.Component] -
* The actual "select" component to use. Defaults to `SelectNext`.
* The actual "select" component to use. Defaults to `Select`.
*/
const createComponent = (props = {}, options = {}) => {
const {
paddingTop = 0,
optionsChildrenAsCallback = true,
Component = SelectNext,
Component = Select,
} = options;
const container = document.createElement('div');
container.style.paddingTop = `${paddingTop}px`;
Expand Down Expand Up @@ -99,29 +99,18 @@ describe('SelectNext', () => {
const clickOption = (wrapper, id) =>
wrapper.find(`[data-testid="option-${id}"]`).simulate('click');

[
{
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);
it('changes selected value when an option is clicked', () => {
const onChange = sinon.stub();
const wrapper = createComponent({ onChange });

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 @@ -332,12 +321,11 @@ describe('SelectNext', () => {
});
});

context('when Option is rendered outside of SelectNext', () => {
context('when Option is rendered outside of Select', () => {
it('throws an error', () => {
assert.throws(
() =>
mount(<SelectNext.Option value="1">{() => '1'}</SelectNext.Option>),
'Select.Option can only be used as Select child',
() => mount(<Select.Option value="1">{() => '1'}</Select.Option>),
'Select.Option can only be used as Select or MultiSelect child',
);
});
});
Expand Down Expand Up @@ -406,16 +394,22 @@ describe('SelectNext', () => {
});
});

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

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

toggleListbox(wrapper);
assert.isFalse(isListboxClosed(wrapper));
Expand All @@ -426,43 +420,32 @@ describe('SelectNext', () => {
assert.isFalse(isListboxClosed(wrapper));
});

[
{
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,
);
it('allows multiple items to be selected', () => {
const onChange = sinon.stub();
const wrapper = createComponent(
{
value: [items[0], items[2]],
onChange,
},
{ Component: MultiSelect },
);

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', () => {
const onChange = sinon.stub();
const wrapper = createComponent({
multiple: true,
value: [items[0], items[2]],
onChange,
});
const wrapper = createComponent(
{
value: [items[0], items[2]],
onChange,
},
{ Component: MultiSelect },
);

toggleListbox(wrapper);
clickOption(wrapper, 3);
Expand All @@ -473,11 +456,13 @@ describe('SelectNext', () => {

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,
});
const wrapper = createComponent(
{
value: [items[0], items[2]],
onChange,
},
{ Component: MultiSelect },
);

toggleListbox(wrapper);
wrapper.find(`[data-testid="reset-option"]`).simulate('click');
Expand Down Expand Up @@ -510,16 +495,15 @@ describe('SelectNext', () => {
},
},
{
name: 'Open Multi-Select listbox',
name: 'Open MultiSelect listbox',
content: () => {
const wrapper = createComponent(
{
buttonContent: 'Select',
'aria-label': 'Select',
value: [items[1], items[3]],
multiple: true,
},
{ optionsChildrenAsCallback: false },
{ optionsChildrenAsCallback: false, Component: MultiSelect },
);
toggleListbox(wrapper);

Expand Down
21 changes: 10 additions & 11 deletions src/pattern-library/components/patterns/input/InputGroupPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
CopyIcon,
ArrowLeftIcon,
ArrowRightIcon,
SelectNext,
Select,
} from '../../../../';
import Library from '../../Library';

Expand Down Expand Up @@ -52,7 +52,10 @@ export default function InputGroupPage() {
<code>IconButton</code>
</li>
<li>
<code>SelectNext</code>
<code>Select</code>
</li>
<li>
<code>MultiSelect</code>
</li>
</ul>
<Library.Demo
Expand All @@ -66,15 +69,11 @@ export default function InputGroupPage() {
title="Previous"
variant="dark"
/>
<SelectNext
value="1"
buttonContent="Apple"
onChange={() => {}}
>
<SelectNext.Option value="1">Apple</SelectNext.Option>
<SelectNext.Option value="2">Banana</SelectNext.Option>
<SelectNext.Option value="3">Cherries</SelectNext.Option>
</SelectNext>
<Select value="1" buttonContent="Apple" onChange={() => {}}>
<Select.Option value="1">Apple</Select.Option>
<Select.Option value="2">Banana</Select.Option>
<Select.Option value="3">Cherries</Select.Option>
</Select>
<IconButton
icon={ArrowRightIcon}
title="Next"
Expand Down
Loading