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.

Sign up for GitHub

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

Jump to bottom

breaking(Dropdown): remove hidden select #1730

Merged
merged 2 commits into from
Jun 15, 2017
Merged

breaking(Dropdown): remove hidden select #1730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78b522c
refactor(Dropdown): remove hidden select
levithomason Jun 1, 2017
c094cf9
test(Dropdown): remove failing aria test
Jun 6, 2017
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
3 changes: 0 additions & 3 deletions src/modules/Dropdown/Dropdown.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,6 @@ export interface DropdownProps {
/** A selection dropdown can allow multiple selections. */
multiple?: boolean;

/** Name of the hidden input which holds the value. */
name?: string;

/** Message to display when there are no results. */
noResultsMessage?: string;

Expand Down
27 changes: 1 addition & 26 deletions src/modules/Dropdown/Dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,6 @@ export default class Dropdown extends Component {
/** A selection dropdown can allow multiple selections. */
multiple: PropTypes.bool,

/** Name of the hidden input which holds the value. */
name: PropTypes.string,

/** Message to display when there are no results. */
noResultsMessage: PropTypes.string,

Expand Down Expand Up @@ -1060,28 +1057,8 @@ export default class Dropdown extends Component {
return <div className={classes}>{_text}</div>
}

renderHiddenInput = () => {
debug('renderHiddenInput()')
const { value } = this.state
const { multiple, name, options, selection } = this.props
debug(`name: ${name}`)
debug(`selection: ${selection}`)
debug(`value: ${value}`)
if (!selection) return null

// a dropdown without an active item will have an empty string value
return (
<select type='hidden' aria-hidden='true' name={name} value={value} multiple={multiple}>
<option value='' />
{_.map(options, (option, i) => (
<option key={getKeyOrValue(option.key, option.value)} value={option.value}>{option.text}</option>
))}
</select>
)
}

renderSearchInput = () => {
const { disabled, search, name, tabIndex } = this.props
const { disabled, search, tabIndex } = this.props
const { searchQuery } = this.state

if (!search) return null
Expand All @@ -1107,7 +1084,6 @@ export default class Dropdown extends Component {
aria-autocomplete='list'
onChange={this.handleSearchChange}
className='search'
name={[name, 'search'].join('-')}
autoComplete='off'
tabIndex={computedTabIndex}
style={{ width: searchWidth }}
Expand Down Expand Up @@ -1285,7 +1261,6 @@ export default class Dropdown extends Component {
tabIndex={computedTabIndex}
ref={this.handleRef}
>
{this.renderHiddenInput()}
{this.renderLabels()}
{this.renderSearchInput()}
{this.renderSearchSizer()}
Expand Down
93 changes: 16 additions & 77 deletions test/specs/modules/Dropdown/Dropdown-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,6 @@ describe('Dropdown', () => {
wrapperMount(<Dropdown />)
wrapper.find('div').at(0).should.have.prop('role', 'listbox')
})
it('should label selection dropdown with aria-hidden=true', () => {
wrapperMount(<Dropdown selection />)
wrapper.find('select').at(0).should.have.prop('aria-hidden', 'true')
})
Copy link
Member

Choose a reason for hiding this comment

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

@levithomason I removed this failing test, now they're passing.

it('should label search dropdown as a combobox', () => {
wrapperMount(<Dropdown search />)
wrapper.find('div').at(0).should.have.prop('role', 'combobox')
Expand Down Expand Up @@ -1247,21 +1243,20 @@ describe('Dropdown', () => {
})
describe('removing items', () => {
it('calls onChange without the clicked value', () => {
const name = 'my-dropdown'
const value = _.map(options, 'value')
const randomIndex = _.random(options.length - 1)
const randomValue = value[randomIndex]
const expected = _.without(value, randomValue)
const spy = sandbox.spy()
wrapperMount(<Dropdown options={options} selection name={name} value={value} multiple onChange={spy} />)
wrapperMount(<Dropdown options={options} selection value={value} multiple onChange={spy} />)

wrapper
.find('.delete.icon')
.at(randomIndex)
.simulate('click')

spy.should.have.been.calledOnce()
spy.should.have.been.calledWithMatch({}, { name, value: expected })
spy.should.have.been.calledWithMatch({}, { value: expected })
})
})
})
Expand All @@ -1283,25 +1278,23 @@ describe('Dropdown', () => {
spy.should.not.have.been.called()
})
it('removes the last item when there is no search query', () => {
const name = 'my-dropdown'
const value = _.map(options, 'value')
const expected = _.dropRight(value)
wrapperMount(<Dropdown options={options} selection name={name} value={value} multiple search onChange={spy} />)
wrapperMount(<Dropdown options={options} selection value={value} multiple search onChange={spy} />)

// open
wrapper.simulate('click')

domEvent.keyDown(document, { key: 'Backspace' })

spy.should.have.been.calledOnce()
spy.should.have.been.calledWithMatch({}, { name, value: expected })
spy.should.have.been.calledWithMatch({}, { value: expected })
})
it('removes the last item when there is no search query when uncontrolled', () => {
const name = 'my-dropdown'
const value = _.map(options, 'value')
const expected = _.dropRight(value)
wrapperMount(
<Dropdown options={options} selection name={name} defaultValue={value} multiple search onChange={spy} />
<Dropdown options={options} selection defaultValue={value} multiple search onChange={spy} />
)

// open
Expand All @@ -1310,7 +1303,7 @@ describe('Dropdown', () => {
domEvent.keyDown(document, { key: 'Backspace' })

spy.should.have.been.calledOnce()
spy.should.have.been.calledWithMatch({}, { name, value: expected })
spy.should.have.been.calledWithMatch({}, { value: expected })

wrapper
.state('value')
Expand Down Expand Up @@ -1351,38 +1344,35 @@ describe('Dropdown', () => {
})

it('is called with event and value on item click', () => {
const name = 'my-dropdown'
const randomIndex = _.random(options.length - 1)
const randomValue = options[randomIndex].value
wrapperMount(<Dropdown options={options} selection onChange={spy} name={name} />)
wrapperMount(<Dropdown options={options} selection onChange={spy} />)
.simulate('click')
.find('DropdownItem')
.at(randomIndex)
.simulate('click')

spy.should.have.been.calledOnce()
spy.should.have.been.calledWithMatch({}, { name, value: randomValue })
spy.should.have.been.calledWithMatch({}, { value: randomValue })
})
it('is called with event and value when pressing enter on a selected item', () => {
const name = 'my-dropdown'
const firstValue = options[0].value
wrapperMount(<Dropdown options={options} selection onChange={spy} name={name} />)
wrapperMount(<Dropdown options={options} selection onChange={spy} />)
.simulate('click')

domEvent.keyDown(document, { key: 'Enter' })

spy.should.have.been.calledOnce()
spy.should.have.been.calledWithMatch({}, { name, value: firstValue })
spy.should.have.been.calledWithMatch({}, { value: firstValue })
})
it('is called with event and value when blurring', () => {
const name = 'my-dropdown'
const firstValue = options[0].value
wrapperMount(<Dropdown options={options} selection onChange={spy} name={name} />)
wrapperMount(<Dropdown options={options} selection onChange={spy} />)
.simulate('focus') // open, highlights first item
.simulate('blur') // blur should activate selected item

spy.should.have.been.calledOnce()
spy.should.have.been.calledWithMatch({}, { name, value: firstValue })
spy.should.have.been.calledWithMatch({}, { value: firstValue })
})
it('is not called on blur when closed', () => {
wrapperMount(<Dropdown options={options} selection open={false} onChange={spy} />)
Expand Down Expand Up @@ -1513,48 +1503,6 @@ describe('Dropdown', () => {
})
})

describe('selection', () => {
it('does not add a hidden select by default', () => {
wrapperMount(<Dropdown />)
.should.not.have.descendants('select[type="hidden"]')
})
it('adds a hidden select', () => {
wrapperShallow(<Dropdown options={options} selection />)
.should.have.exactly(1).descendants('select[type="hidden"]')
})
it('passes the name prop to the hidden select', () => {
wrapperShallow(<Dropdown name='foo' options={options} selection />)
.find('select[type="hidden"]').should.have.prop('name', 'foo')
})
it('passes the value prop to the hidden select', () => {
const value = _.values(options)

wrapperShallow(<Dropdown name='foo' value={value} options={options} selection />)
.find('select[type="hidden"][name="foo"]').should.have.prop('value', value)
})
it('passes the multiple prop to the hidden select', () => {
const selector = 'select[type="hidden"][name="foo"]'

wrapperShallow(<Dropdown name='foo' multiple options={options} selection />)
.find(selector).should.have.prop('multiple', true)

wrapperShallow(<Dropdown name='foo' multiple={false} options={options} selection />)
.find(selector).should.have.prop('multiple', false)
})
it('adds options to the hidden select', () => {
wrapperShallow(<Dropdown options={options} selection />)
.should.have.exactly(options.length + 1).descendants('option')

options.forEach((opt, i) => {
// index is incremented to exclude the empty option value
const optionElement = wrapper.find('option').at(i + 1)

optionElement.should.have.prop('value', opt.value)
optionElement.should.have.text(opt.text)
})
})
})

describe('search', () => {
it('does not add a search input when not defined', () => {
wrapperShallow(<Dropdown options={options} selection />)
Expand All @@ -1566,13 +1514,6 @@ describe('Dropdown', () => {
.should.have.exactly(1).descendants('input.search')
})

it('adds the name prop to the search input', () => {
wrapperShallow(<Dropdown name='foo' options={options} selection search />)
.should.have.descendants('input.search')

wrapper.find('input.search').should.have.prop('name', 'foo-search')
})

it('sets focus to the search input on open', () => {
wrapperMount(<Dropdown options={options} selection search />)
.simulate('click')
Expand Down Expand Up @@ -2122,9 +2063,8 @@ describe('Dropdown', () => {

it('calls onAddItem prop when clicking new value', () => {
const spy = sandbox.spy()
const name = 'my-dropdown'
const search = wrapperMount(
<Dropdown options={customOptions} selection search allowAdditions onAddItem={spy} name={name} />
<Dropdown options={customOptions} selection search allowAdditions onAddItem={spy} />
)
.find('input.search')

Expand All @@ -2136,22 +2076,21 @@ describe('Dropdown', () => {
.simulate('click')

spy.should.have.been.calledOnce()
spy.should.have.been.calledWithMatch({}, { name, value: 'boo' })
spy.should.have.been.calledWithMatch({}, { value: 'boo' })
})

it('calls onAddItem prop when pressing enter on new value', () => {
const spy = sandbox.spy()
const name = 'my-dropdown'
const search = wrapperMount(
<Dropdown options={customOptions} selection search allowAdditions onAddItem={spy} name={name} />
<Dropdown options={customOptions} selection search allowAdditions onAddItem={spy} />
)
.find('input.search')

search.simulate('change', { target: { value: 'boo' } })
domEvent.keyDown(document, { key: 'Enter' })

spy.should.have.been.calledOnce()
spy.should.have.been.calledWithMatch({}, { name, value: 'boo' })
spy.should.have.been.calledWithMatch({}, { value: 'boo' })
})
})

Expand Down