Skip to content

Commit 5934ad1

Browse files
mdodgelookermdodgelooker
authored
ButtonGroup & ButtonToggle: add options, use buttons (#1015)
1 parent 224ccc3 commit 5934ad1

File tree

13 files changed

+915
-531
lines changed

13 files changed

+915
-531
lines changed

‎CHANGELOG.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1010
### Added
1111

1212
- `TreeGroup` component
13+
- `ButtonGroup` and `ButtonToggle` now support `options` as an alternative to nested `ButtonItem` children
14+
15+
### Fixed
16+
17+
- `ButtonGroup` and `ButtonToggle` accessibility issues due to hidden `input`s (they now render a list of `button`s instead)
18+
19+
### Removed
20+
21+
- `ButtonGroup` and `ButtonToggle` no longer support an uncontrolled version, since they now render a list of `button`s instead of checkbox and radio groups.
1322

1423
## [0.8.2]
1524

‎packages/components/src/Button/ButtonGroup.test.tsx‎

Lines changed: 86 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -25,91 +25,96 @@
2525
*/
2626

2727
import '@testing-library/jest-dom/extend-expect'
28-
// import { fireEvent } from '@testing-library/react'
29-
import React from 'react'
30-
import { Simulate } from 'react-dom/test-utils'
28+
import { fireEvent, screen } from '@testing-library/react'
29+
import React, { useState } from 'react'
3130

3231
import { renderWithTheme } from '@looker/components-test-utils'
33-
import { theme } from '@looker/design-tokens'
3432
import { ButtonGroup } from './ButtonGroup'
3533
import { ButtonItem } from './ButtonItem'
3634

37-
const requiredProps = {
38-
message: 'Foo',
39-
onConfirm: jest.fn(),
40-
title: 'Bar',
41-
}
42-
43-
const optionalProps = {
44-
cancelLabel: 'Dont Delete',
45-
confirmLabel: 'Delete',
46-
message: 'This is permanent',
47-
onCancel: jest.fn(),
48-
title: 'Delete the thing?',
49-
}
50-
51-
afterEach(() => {
52-
requiredProps.onConfirm.mockClear()
53-
optionalProps.onCancel.mockClear()
54-
})
55-
56-
test('<ButtonGroup/> uncontrolled', () => {
57-
const onChangeMock = jest.fn()
58-
const { container, getByLabelText, getByText } = renderWithTheme(
59-
<form>
60-
<ButtonGroup name="fruits" label="Fruits">
61-
<ButtonItem selected>Apples</ButtonItem>
62-
<ButtonItem onChange={onChangeMock}>Oranges</ButtonItem>
63-
<ButtonItem>Bananas</ButtonItem>
35+
describe('ButtonGroup', () => {
36+
test('controlled', () => {
37+
function TestComponent() {
38+
const [value, setValue] = useState(['Bananas'])
39+
return (
40+
<ButtonGroup value={value} onChange={setValue}>
41+
<ButtonItem>Apples</ButtonItem>
42+
<ButtonItem>Oranges</ButtonItem>
43+
<ButtonItem>Bananas</ButtonItem>
44+
</ButtonGroup>
45+
)
46+
}
47+
renderWithTheme(<TestComponent />)
48+
49+
expect(screen.getByRole('group')).toMatchSnapshot()
50+
51+
const apples = screen.getByText('Apples')
52+
const bananas = screen.getByText('Bananas')
53+
const oranges = screen.getByText('Oranges')
54+
55+
expect(apples).toHaveAttribute('aria-pressed', 'false')
56+
expect(bananas).toHaveAttribute('aria-pressed', 'true')
57+
expect(oranges).toHaveAttribute('aria-pressed', 'false')
58+
59+
fireEvent.click(apples)
60+
expect(apples).toHaveAttribute('aria-pressed', 'true')
61+
expect(bananas).toHaveAttribute('aria-pressed', 'true')
62+
expect(oranges).toHaveAttribute('aria-pressed', 'false')
63+
64+
fireEvent.click(bananas)
65+
expect(apples).toHaveAttribute('aria-pressed', 'true')
66+
expect(bananas).toHaveAttribute('aria-pressed', 'false')
67+
expect(oranges).toHaveAttribute('aria-pressed', 'false')
68+
69+
fireEvent.click(oranges)
70+
expect(apples).toHaveAttribute('aria-pressed', 'true')
71+
expect(bananas).toHaveAttribute('aria-pressed', 'false')
72+
expect(oranges).toHaveAttribute('aria-pressed', 'true')
73+
})
74+
75+
test('options', () => {
76+
const options = [
77+
{ label: 'Smoked Gouda', value: 'Gouda' },
78+
{ value: 'Cheddar' },
79+
{ disabled: true, value: 'Swiss' },
80+
]
81+
const onChangeMock = jest.fn()
82+
renderWithTheme(
83+
<ButtonGroup
84+
options={options}
85+
value={['Cheddar']}
86+
onChange={onChangeMock}
87+
/>
88+
)
89+
const goudaButton = screen.getByText('Smoked Gouda')
90+
const cheddarButton = screen.getByText('Cheddar')
91+
const swissButton = screen.getByText('Swiss')
92+
93+
expect(goudaButton).toHaveAttribute('aria-pressed', 'false')
94+
expect(cheddarButton).toHaveAttribute('aria-pressed', 'true')
95+
expect(swissButton).toHaveAttribute('aria-pressed', 'false')
96+
expect(swissButton).toBeDisabled()
97+
98+
fireEvent.click(goudaButton)
99+
expect(onChangeMock).toBeCalledWith(['Cheddar', 'Gouda'])
100+
onChangeMock.mockClear()
101+
fireEvent.click(cheddarButton)
102+
expect(onChangeMock).toHaveBeenCalledWith([])
103+
onChangeMock.mockClear()
104+
// Disabled
105+
fireEvent.click(swissButton)
106+
expect(onChangeMock).not.toHaveBeenCalled()
107+
})
108+
109+
test('disabled', () => {
110+
const onChangeMock = jest.fn()
111+
renderWithTheme(
112+
<ButtonGroup disabled onChange={onChangeMock}>
113+
<ButtonItem>Apples</ButtonItem>
64114
</ButtonGroup>
65-
</form>
66-
)
67-
68-
const apples = getByLabelText('Apples')
69-
const oranges = getByLabelText('Oranges')
70-
71-
expect(apples).toHaveAttribute('checked')
72-
expect(oranges).not.toHaveAttribute('checked')
73-
expect(getByText('Apples')).toHaveStyle(`color: ${theme.colors.key}`)
74-
expect(getByText('Oranges')).toHaveStyle(`color: ${theme.colors.text3}`)
75-
76-
expect(getByLabelText('Bananas')).toBeInTheDocument()
77-
78-
Simulate.change(apples)
79-
expect(onChangeMock).not.toHaveBeenCalled()
80-
81-
Simulate.change(oranges)
82-
expect(onChangeMock).toHaveBeenCalled()
83-
84-
const form = container.querySelector('form')
85-
if (form) {
86-
const formData = new FormData(form)
87-
expect(formData.get('fruits')).toEqual('Oranges')
88-
}
89-
})
90-
91-
test('<ButtonGroup/> controlled', () => {
92-
const onChangeMock = jest.fn()
93-
const { getByLabelText } = renderWithTheme(
94-
<ButtonGroup value={['Bananas']} onChange={onChangeMock}>
95-
<ButtonItem>Apples</ButtonItem>
96-
<ButtonItem>Oranges</ButtonItem>
97-
<ButtonItem>Bananas</ButtonItem>
98-
</ButtonGroup>
99-
)
100-
101-
const apples = getByLabelText('Apples')
102-
const bananas = getByLabelText('Bananas')
103-
const oranges = getByLabelText('Oranges')
104-
105-
expect(bananas).toHaveAttribute('checked')
106-
expect(oranges).not.toHaveAttribute('checked')
107-
108-
Simulate.change(apples)
109-
Simulate.change(bananas)
110-
Simulate.change(oranges)
111-
expect(onChangeMock).toHaveBeenCalledTimes(3)
112-
expect(onChangeMock.mock.calls[0][0]).toEqual(['Bananas', 'Apples'])
113-
expect(onChangeMock.mock.calls[1][0]).toEqual([])
114-
expect(onChangeMock.mock.calls[2][0]).toEqual(['Bananas', 'Oranges'])
115+
)
116+
const applesButton = screen.getByText('Apples')
117+
fireEvent.click(applesButton)
118+
expect(onChangeMock).not.toHaveBeenCalled()
119+
})
115120
})

‎packages/components/src/Button/ButtonGroup.tsx‎

Lines changed: 25 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -25,63 +25,53 @@
2525
*/
2626

2727
import xor from 'lodash/xor'
28-
import React, { ChangeEvent, forwardRef, Ref } from 'react'
28+
import React, { forwardRef, MouseEvent, Ref } from 'react'
2929
import styled from 'styled-components'
30-
import { useControlWarn, useID } from '../utils'
31-
import { ButtonItem, ButtonItemLabel } from './ButtonItem'
30+
import { ButtonItem } from './ButtonItem'
3231
import { ButtonGroupOrToggleBaseProps, ButtonSet } from './ButtonSet'
3332

3433
const ButtonGroupLayout = forwardRef(
3534
(
36-
{
37-
onChange,
38-
value,
39-
...props
40-
}: Omit<ButtonGroupOrToggleBaseProps<string[]>, 'nullable'>,
35+
{ onChange, value = [], ...props }: ButtonGroupOrToggleBaseProps,
4136
ref: Ref<HTMLDivElement>
4237
) => {
43-
const name = useID(props.name)
44-
45-
const isControlled = useControlWarn({
46-
controllingProps: ['onChange', 'value'],
47-
isControlledCheck: () => onChange !== undefined,
48-
name: 'ButtonGroup',
49-
})
50-
51-
function handleChange(e?: ChangeEvent<HTMLInputElement>) {
52-
// event is only ever missing for nullable ButtonToggle
53-
if (e) {
54-
const newValue = xor(value, [e.target.value])
55-
onChange && onChange(newValue)
38+
function handleItemClick(e: MouseEvent<HTMLButtonElement>) {
39+
const newValue = xor(value, [e.currentTarget.value])
40+
if (onChange) {
41+
onChange(newValue)
5642
}
5743
}
5844

5945
return (
6046
<ButtonSet
6147
{...props}
6248
ref={ref}
63-
{...(isControlled
64-
? {
65-
onChange: handleChange,
66-
value,
67-
}
68-
: { name })}
49+
value={value}
50+
onItemClick={handleItemClick}
6951
/>
7052
)
7153
}
7254
)
7355

74-
export const ButtonGroup = styled(ButtonGroupLayout)`
75-
margin: -${({ theme }) => theme.space.xxxsmall};
56+
ButtonGroupLayout.displayName = 'ButtonGroupLayout'
7657

58+
export const ButtonGroup = styled(ButtonGroupLayout)`
7759
${ButtonItem} {
60+
border: 1px solid ${({ theme }) => theme.colors.keyAccent};
7861
border-radius: ${({ theme }) => theme.radii.medium};
79-
height: 36px;
80-
margin: ${({ theme }) => theme.space.xxxsmall};
81-
}
62+
margin-right: ${({ theme }) => theme.space.xxsmall};
63+
&:last-child {
64+
margin-right: 0;
65+
}
8266
83-
${ButtonItemLabel} {
84-
border-style: solid;
85-
border-width: 1px;
67+
&[aria-pressed='false']:not(:hover) {
68+
background: ${({ theme }) => theme.colors.background};
69+
}
70+
}
71+
&.wrapping {
72+
margin: -${({ theme }) => theme.space.xxxsmall};
73+
${ButtonItem} {
74+
margin: ${({ theme }) => theme.space.xxxsmall};
75+
}
8676
}
8777
`

0 commit comments

Comments
 (0)