Skip to content

Commit

Permalink
fix(Select): Behave as controlled component
Browse files Browse the repository at this point in the history
Previously, the Select `value` prop was only setting the initial value of the component, which was unexpected for a controlled component. In particular, this resulted in unexpected behaviour when the value or children changed externally.

This makes the component controlled when `value` is set so it behaves as expected in these scenarios. `initialValue` was also added for uncontrolled scenarios.

This also brings the component in line with recent changes to Autocomplete.
  • Loading branch information
jpveooys committed Sep 23, 2022
1 parent 2f0719a commit acc08ef
Show file tree
Hide file tree
Showing 5 changed files with 144 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@ export interface AutocompleteProps extends SelectBaseProps {
* Called when the input loses focus.
*/
onBlur?: (event: React.FocusEvent) => void
/**
* The initially selected item when the component is uncontrolled.
*/
initialValue?: string | null
}

export const Autocomplete: React.FC<AutocompleteProps> = ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ Disabled.args = {
export const NoClearButton = Template.bind({})
NoClearButton.args = {
hideClearButton: true,
value: 'two',
initialValue: 'two',
}

export const Open = Template.bind({})
Expand All @@ -95,7 +95,7 @@ WithError.args = {

export const WithValue = Template.bind({})
WithValue.args = {
value: 'two',
initialValue: 'two',
}

export const WithIconsAndBadges = TemplateWithIconsAndBadges.bind({})
128 changes: 128 additions & 0 deletions packages/react-component-library/src/components/Select/Select.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,134 @@ describe('Select', () => {
})
})

describe('when the component is controlled with "One" selected', () => {
const ControlledSelect = () => {
const [value, setValue] = React.useState<string | null>('one')

return (
<Select
id="select-id"
label="Label"
onChange={(newValue) => setValue(newValue)}
value={value}
>
<SelectOption value="one">One</SelectOption>
<SelectOption value="two">Two</SelectOption>
</Select>
)
}

beforeEach(() => {
wrapper = render(<ControlledSelect />)
})

it('has the value "One"', () => {
expect(wrapper.getByTestId('select-input')).toHaveValue('One')
})

describe('when the menu is opened and the second item clicked', () => {
beforeEach(async () => {
await userEvent.click(wrapper.getByTestId('select-arrow-button'))
return userEvent.click(wrapper.getByText('Two'))
})

it('has the value "Two"', () => {
expect(wrapper.getByTestId('select-input')).toHaveValue('Two')
})
})
})

describe('when the component is uncontrolled with "One" selected', () => {
beforeEach(() => {
wrapper = render(
<Select
id="select-id"
label="Label"
onChange={jest.fn()}
initialValue="one"
>
<SelectOption value="one">One</SelectOption>
<SelectOption value="two">Two</SelectOption>
</Select>
)
})

it('has the value "One"', () => {
expect(wrapper.getByTestId('select-input')).toHaveValue('One')
})

describe('when the menu is opened and the second item clicked', () => {
beforeEach(async () => {
await userEvent.click(wrapper.getByTestId('select-arrow-button'))
return userEvent.click(wrapper.getByText('Two'))
})

it('has the value "Two"', () => {
expect(wrapper.getByTestId('select-input')).toHaveValue('Two')
})
})
})

describe('when options are added after the initial render', () => {
beforeEach(() => {
wrapper = render(
<Select id="select-id" label="Label" onChange={jest.fn()} value="one">
<>{}</>
</Select>
)
wrapper.rerender(
<Select id="select-id" label="Label" onChange={jest.fn()} value="one">
<SelectOption value="one">One</SelectOption>
<SelectOption value="two">Two</SelectOption>
</Select>
)
return userEvent.click(wrapper.getByTestId('select-arrow-button'))
})

it('displays the items', () => {
const options = wrapper.getAllByTestId('select-option')

expect(options[0]).toHaveTextContent('One')
expect(options[1]).toHaveTextContent('Two')
expect(options).toHaveLength(2)
})

it('has the correct value', () => {
expect(wrapper.getByTestId('select-input')).toHaveValue('One')
})
})

describe('when options are added while the menu is open', () => {
beforeEach(async () => {
wrapper = render(
<Select id="select-id" label="Label" onChange={jest.fn()} value="one">
<>{}</>
</Select>
)

await userEvent.click(wrapper.getByTestId('select-arrow-button'))

wrapper.rerender(
<Select id="select-id" label="Label" onChange={jest.fn()} value="one">
<SelectOption value="one">One</SelectOption>
<SelectOption value="two">Two</SelectOption>
</Select>
)
})

it('displays the items', () => {
const options = wrapper.getAllByTestId('select-option')

expect(options[0]).toHaveTextContent('One')
expect(options[1]).toHaveTextContent('Two')
expect(options).toHaveLength(2)
})

it('has the correct value', () => {
expect(wrapper.getByTestId('select-input')).toHaveValue('One')
})
})

describe('when the default `id` is used and the arrow button is clicked', () => {
let initialId: string

Expand Down
12 changes: 10 additions & 2 deletions packages/react-component-library/src/components/Select/Select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ export const Select: React.FC<SelectBaseProps> = ({
children,
id: externalId,
initialIsOpen,
initialValue,
onChange,
value = null,
value,
...rest
}) => {
const id = useExternalId('select', externalId)
Expand All @@ -32,6 +33,8 @@ export const Select: React.FC<SelectBaseProps> = ({
filteredItems.map((item) => [item.props.value, item])
)

const isControlled = value !== undefined

const {
getItemProps,
getMenuProps,
Expand All @@ -44,11 +47,16 @@ export const Select: React.FC<SelectBaseProps> = ({
} = useSelect<string>({
itemToString: (item) => itemToString(item, itemsMap),
initialIsOpen,
initialSelectedItem: getSelectedItem(value, itemsMap),
items,
onSelectedItemChange: ({ selectedItem: newItem }) => {
onChange?.(newItem ?? null)
},
...{
[isControlled ? 'selectedItem' : 'initialSelectedItem']: getSelectedItem(
isControlled ? value : initialValue,
itemsMap
),
},
})

const { onInputFocusHandler } = useMenuVisibility(isOpen, openMenu)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ export interface SelectBaseProps extends ComponentWithClass {
* Toggles whether the list is open on first render.
*/
initialIsOpen?: boolean
/**
* The initially selected item when the component is uncontrolled.
*/
initialValue?: string | null
/**
* Toggles whether the component is disabled or not (preventing user interaction).
*/
Expand Down

0 comments on commit acc08ef

Please sign in to comment.