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

fix(Dropdown): fix minCharacter prop behaviour #1722

Merged
merged 3 commits into from
Jun 22, 2017
Merged

fix(Dropdown): fix minCharacter prop behaviour #1722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42c14ce
wip
May 31, 2017
347e437
Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…
Jun 6, 2017
5f70d73
test(Dropdown): add missing tests
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
105 changes: 50 additions & 55 deletions src/modules/Dropdown/Dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,19 +334,19 @@ export default class Dropdown extends Component {
static defaultProps = {
additionLabel: 'Add ',
additionPosition: 'top',
closeOnBlur: true,
icon: 'dropdown',
minCharacters: 1,
noResultsMessage: 'No results found.',
openOnFocus: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

Only prop sort there.

renderLabel: ({ text }) => text,
selectOnBlur: true,
openOnFocus: true,
closeOnBlur: true,
minCharacters: 1,
}

static autoControlledProps = [
'open',
'value',
'selectedLabel',
'value',
]

static _meta = {
Expand Down Expand Up @@ -420,9 +420,11 @@ export default class Dropdown extends Component {
if (!prevState.focus && this.state.focus) {
debug('dropdown focused')
if (!this.isMouseDown) {
const { openOnFocus } = this.props
const { minCharacters, openOnFocus, search } = this.props
const openable = !search || (search && minCharacters === 1)

debug('mouse is not down, opening')
if (openOnFocus) this.open()
if (openOnFocus && openable) this.open()
Copy link
Member Author

Choose a reason for hiding this comment

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

Defines behaviour with focus.

}
if (!this.state.open) {
document.addEventListener('keydown', this.openOnArrow)
Expand Down Expand Up @@ -492,10 +494,8 @@ export default class Dropdown extends Component {
// onChange needs to receive a value
// can't rely on props.value if we are controlled
handleChange = (e, value) => {
debug('handleChange()')
debug(value)
const { onChange } = this.props
if (onChange) onChange(e, { ...this.props, value })
debug('handleChange()', value)
_.invoke(this.props, 'onChange', e, { ...this.props, value })
}

closeOnChange = (e) => {
Expand Down Expand Up @@ -580,11 +580,13 @@ export default class Dropdown extends Component {
}

selectItemOnEnter = (e) => {
debug('selectItemOnEnter()')
debug(keyboardKey.getName(e))
debug('selectItemOnEnter()', keyboardKey.getName(e))
const { search } = this.props

if (keyboardKey.getCode(e) !== keyboardKey.Enter) return
e.preventDefault()

if (search && _.isEmpty(this.getMenuOptions())) return
Copy link
Member Author

Choose a reason for hiding this comment

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

Disables Enter key when nothing find.

this.makeSelectedItemActive(e)
this.closeOnChange(e)
}
Expand Down Expand Up @@ -624,29 +626,34 @@ export default class Dropdown extends Component {

handleMouseDown = (e) => {
debug('handleMouseDown()')
const { onMouseDown } = this.props
if (onMouseDown) onMouseDown(e, this.props)

this.isMouseDown = true
_.invoke(this.props, 'onMouseDown', e, this.props)
// Do not access document when server side rendering
if (!isBrowser) return
document.addEventListener('mouseup', this.handleDocumentMouseUp)
if (isBrowser) document.addEventListener('mouseup', this.handleDocumentMouseUp)
Copy link
Member Author

Choose a reason for hiding this comment

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

Minor refactor.

}

handleDocumentMouseUp = () => {
debug('handleDocumentMouseUp()')

this.isMouseDown = false
// Do not access document when server side rendering
if (!isBrowser) return
document.removeEventListener('mouseup', this.handleDocumentMouseUp)
if (isBrowser) document.removeEventListener('mouseup', this.handleDocumentMouseUp)
Copy link
Member Author

Choose a reason for hiding this comment

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

Minor refactor.

}

handleClick = (e) => {
handleClick = e => {
debug('handleClick()', e)
const { onClick } = this.props
if (onClick) onClick(e, this.props)

const { minCharacters, search } = this.props
const { open, searchQuery } = this.state

_.invoke(this.props, 'onClick', e, this.props)
// prevent closeOnDocumentClick()
e.stopPropagation()
this.toggle(e)

if (!search) return this.toggle(e)
if (open) return
if (searchQuery.length >= minCharacters || minCharacters === 1) this.open(e)
Copy link
Member Author

Choose a reason for hiding this comment

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

Update behaviour with click on search input.

}

handleItemClick = (e, item) => {
Expand Down Expand Up @@ -683,14 +690,15 @@ export default class Dropdown extends Component {

handleFocus = (e) => {
debug('handleFocus()')
const { onFocus } = this.props
const { focus } = this.state

if (focus) return
if (onFocus) onFocus(e, this.props)

_.invoke(this.props, 'onFocus', e, this.props)
this.setState({ focus: true })
}

handleBlur = (e) => {
handleBlur = e => {
debug('handleBlur()')

// Heads up! Don't remove this.
Expand All @@ -709,26 +717,28 @@ export default class Dropdown extends Component {
this.setState({ focus: false, searchQuery: '' })
}

handleSearchChange = (e) => {
debug('handleSearchChange()')
debug(e.target.value)
handleSearchChange = e => {
debug('handleSearchChange()', e)
// prevent propagating to this.props.onChange()
e.stopPropagation()
const { search, onSearchChange, minCharacters } = this.props
const { open } = this.state
const newQuery = e.target.value

if (onSearchChange) onSearchChange(e, newQuery)
const { minCharacters } = this.props
const { open } = this.state
const newQuery = _.get(e, 'target.value', '')

if (newQuery.length >= minCharacters) {
// open search dropdown on search query
if (search && newQuery && !open) this.open()
_.invoke(this.props, 'onSearchChange', e, newQuery)
this.setState({
selectedIndex: 0,
searchQuery: newQuery,
})

this.setState({
selectedIndex: 0,
searchQuery: newQuery,
})
// open search dropdown on search query
if (!open && newQuery.length >= minCharacters) {
this.open()
return
}
// close search dropdown if search query is too small
if (open && minCharacters !== 1 && newQuery.length < minCharacters) this.close()
}

// ----------------------------------------
Expand Down Expand Up @@ -1013,22 +1023,7 @@ export default class Dropdown extends Component {
this.setState({ focus: hasFocus })
}

toggle = (e) => {
if (!this.state.open) {
this.open(e)
return
}

const { search } = this.props
const options = this.getMenuOptions()

if (search && _.isEmpty(options)) {
e.preventDefault()
return
}

this.close(e)
}
toggle = e => this.state.open ? this.close(e) : this.open(e)
Copy link
Member Author

@layershifter layershifter Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Method was updated because its logic was moved to handleClick


// ----------------------------------------
// Render
Expand Down
118 changes: 105 additions & 13 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 @@ -1420,6 +1420,76 @@ describe('Dropdown', () => {
})
})

describe('onClick', () => {
it('is called with (event, props)', () => {
const onClick = sandbox.spy()
wrapperMount(<Dropdown onClick={onClick} options={options} />)
wrapper.simulate('click', { stopPropagation: _.noop })

onClick.should.have.been.calledOnce()
onClick.should.have.been.calledWithMatch({ }, { options })
})

it("toggles the dropdown when it's not searchable", () => {
wrapperMount(<Dropdown options={options} />)

wrapper.simulate('click')
dropdownMenuIsOpen()

wrapper.simulate('click')
dropdownMenuIsClosed()
})

it("opens the dropdown when it's searchable, but don't close", () => {
wrapperMount(<Dropdown options={options} search />)

wrapper.simulate('click')
dropdownMenuIsOpen()

wrapper.simulate('click')
dropdownMenuIsOpen()
})

it("don't open the dropdown when it's searchable and minCharacters is more that default value", () => {
wrapperMount(<Dropdown minCharacters={3} options={options} search />)

wrapper.simulate('click')
dropdownMenuIsClosed()
})
})

describe('onFocus', () => {
it('is called with (event, props)', () => {
const onFocus = sandbox.spy()
wrapperMount(<Dropdown onFocus={onFocus} options={options} />)
wrapper.simulate('focus')

onFocus.should.have.been.calledOnce()
onFocus.should.have.been.calledWithMatch({ }, { options })
})

it("opens the dropdown when it's not searchable", () => {
wrapperMount(<Dropdown options={options} />)

wrapper.simulate('focus')
dropdownMenuIsOpen()
})

it("opens the dropdown when it's searchable", () => {
wrapperMount(<Dropdown options={options} search />)

wrapper.simulate('focus')
dropdownMenuIsOpen()
})

it("don't open the dropdown when it's searchable and minCharacters is more that default value", () => {
wrapperMount(<Dropdown minCharacters={3} options={options} search />)

wrapper.simulate('focus')
dropdownMenuIsClosed()
})
})

describe('onSearchChange', () => {
it('is called with (event, value) on search input change', () => {
const spy = sandbox.spy()
Expand All @@ -1430,6 +1500,30 @@ describe('Dropdown', () => {
spy.should.have.been.calledOnce()
spy.should.have.been.calledWithMatch({ target: { value: 'a' } }, 'a')
})

it("don't open the menu on change if query's length is less than minCharacters", () => {
wrapperMount(<Dropdown minCharacters={3} options={options} selection search />)

dropdownMenuIsClosed()

// simulate search with query's length is less than minCharacters
wrapper
.find('input.search')
.simulate('change', { target: { value: 'a' } })

dropdownMenuIsClosed()
})

it("closes the opened menu on change if query's length is less than minCharacters", () => {
wrapperMount(<Dropdown minCharacters={3} options={options} selection search />)
const input = wrapper.find('input.search')

input.simulate('change', { target: { value: 'abc' } })
dropdownMenuIsOpen()

input.simulate('change', { target: { value: 'a' } })
dropdownMenuIsClosed()
})
})

describe('options', () => {
Expand Down Expand Up @@ -1618,19 +1712,6 @@ describe('Dropdown', () => {
dropdownMenuIsOpen()
})

it("Don't open the menu on change if query's length is less than minCharacters", () => {
wrapperMount(<Dropdown options={options} selection search minCharacters={4} />)

dropdownMenuIsClosed()

// simulate search with query's length is less than minCharacters
wrapper
.find('input.search')
.simulate('change', { target: { value: faker.hacker.noun().substring(0, 1) } })

dropdownMenuIsClosed()
})
Copy link
Member Author

Choose a reason for hiding this comment

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

Test was moved to another section.


it('does not call onChange on query change', () => {
const onChangeSpy = sandbox.spy()
wrapperMount(<Dropdown options={options} selection search onChange={onChangeSpy} />)
Expand Down Expand Up @@ -1737,6 +1818,17 @@ describe('Dropdown', () => {
.at(1)
.should.not.have.prop('selected', true)
})

it('does not close the menu when options are empty', () => {
wrapperMount(<Dropdown options={options} search selection />)
wrapper.simulate('click')

wrapper.find('input.search')
.simulate('change', { target: { value: 'foo' } })
domEvent.keyDown(document, { key: 'Enter' })

dropdownMenuIsOpen()
})
})

describe('no results message', () => {
Expand Down