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

fix(Dropdown): UX behavior of selectedItems and scroll for filtered options. #2375

Closed
wants to merge 11 commits into from
Closed

fix(Dropdown): UX behavior of selectedItems and scroll for filtered options. #2375

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
073eb6f
fix(Dropdown): setSelectedIndex on open
francoiscote Dec 12, 2017
bb860f3
fix(Dropdown): remove useless scroll in open handler. this is handled…
francoiscote Dec 12, 2017
466d48d
fix(Dropdown): scrollSelectedItemIntoView when changing searchQuery
francoiscote Dec 12, 2017
2b0279c
fix tests, simplify setSelectedIndex/scrollSelectedItemIntoView
levithomason Feb 14, 2018
28bbf25
add failing dropdown test for selectedIndex/no results
levithomason Feb 14, 2018
3080331
merge master, fix conflicts
levithomason Aug 20, 2018
0813a69
Merge branch 'master' into fix/dropdown-search-selected-item
levithomason Aug 25, 2018
7c67340
fix(Dropdown): close on document with !closeOnBlur
levithomason Aug 25, 2018
2c9bffd
merge master, with conflict fixes
francoiscote May 29, 2019
7855cb2
Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…
layershifter Aug 13, 2020
b39b782
restore irrelevant changes
layershifter Aug 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 26 additions & 7 deletions src/modules/Dropdown/Dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,16 @@ export default class Dropdown extends Component {
])
}

// Value Change
if (prevState.value !== this.state.value) {
this.setSelectedIndex()
}

// Scroll selected item into view
if (prevState.selectedIndex !== this.state.selectedIndex || prevState.searchQuery !== this.state.searchQuery) {
this.scrollSelectedItemIntoView()
}

// opened / closed
if (!prevState.open && this.state.open) {
debug('dropdown opened')
Expand Down Expand Up @@ -594,7 +604,6 @@ export default class Dropdown extends Component {

// notify the onChange prop that the user is trying to change value
this.setValue(newValue)
this.setSelectedIndex(newValue)
this.handleChange(e, newValue)

// Heads up! This event handler should be called after `onChange`
Expand Down Expand Up @@ -632,7 +641,6 @@ export default class Dropdown extends Component {
const newValue = _.dropRight(value)

this.setValue(newValue)
this.setSelectedIndex(newValue)
this.handleChange(e, newValue)
}

Expand Down Expand Up @@ -723,7 +731,6 @@ export default class Dropdown extends Component {

// notify the onChange prop that the user is trying to change value
this.setValue(newValue)
this.setSelectedIndex(value)

const optionSize = _.size(this.getMenuOptions())
if (!multiple || isAdditionItem || optionSize === 1) this.clearSearchQuery()
Expand Down Expand Up @@ -811,7 +818,20 @@ export default class Dropdown extends Component {
// filter by search query
if (search && searchQuery) {
if (_.isFunction(search)) {
filteredOptions = search(filteredOptions, searchQuery)
// poor man's memoization
// if the value, options, and search function are referentially equal to the last call
// just return the last value
const { lastValue, lastOptions, lastSearch } = this.getMenuOptions

if (lastValue === value && lastOptions === options && lastSearch === search) {
filteredOptions = this.getMenuOptions.lastReturn
} else {
this.getMenuOptions.lastValue = value
this.getMenuOptions.lastOptions = options
this.getMenuOptions.lastSearch = search

filteredOptions = search(filteredOptions, searchQuery)
}
Copy link

Choose a reason for hiding this comment

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

This seems unwise to add more state management, especially outside of this.state. I thought my PR addressed this issue quite well by ensuring this.state stayed in sync with prop changes. Did you have a chance to review my PR?

} else {
// remove diacritics on search input and options, if deburr prop is set
const strippedQuery = deburr ? _.deburr(searchQuery) : searchQuery
Expand Down Expand Up @@ -845,6 +865,8 @@ export default class Dropdown extends Component {
else filteredOptions.push(addItem)
}

this.getMenuOptions.lastReturn = filteredOptions

return filteredOptions
}

Expand Down Expand Up @@ -978,7 +1000,6 @@ export default class Dropdown extends Component {
debug('new value:', newValue)

this.setValue(newValue)
this.setSelectedIndex(newValue)
this.handleChange(e, newValue)
}

Expand All @@ -1005,7 +1026,6 @@ export default class Dropdown extends Component {
}

this.setState({ selectedIndex: nextIndex })
this.scrollSelectedItemIntoView()
}

// ----------------------------------------
Expand Down Expand Up @@ -1096,7 +1116,6 @@ export default class Dropdown extends Component {
if (onOpen) onOpen(e, this.props)

this.trySetState({ open: true })
this.scrollSelectedItemIntoView()
}

close = (e) => {
Expand Down
66 changes: 63 additions & 3 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 @@ -34,8 +34,8 @@ const wrapperRender = (...args) => (wrapper = render(...args))
// Options
// ----------------------------------------
const getOptions = (count = 5) => _.times(count, (i) => {
const text = `${i}-${faker.hacker.noun}`
const value = `${i}-${_.snakeCase(text)}`
const text = `${i}-item-text`
const value = `${i}-item-value`
return { text, value }
})

Expand All @@ -57,7 +57,7 @@ const dropdownMenuIsOpen = () => {

const nativeEvent = { nativeEvent: { stopImmediatePropagation: _.noop } }

describe('Dropdown', () => {
describe.only('Dropdown', () => {
beforeEach(() => {
attachTo = undefined
wrapper = undefined
Expand Down Expand Up @@ -1980,6 +1980,66 @@ describe('Dropdown', () => {
wrapper.should.have.state('selectedIndex', 0)
})

it('retains the selected item after searching with no results', () => {
wrapperMount(<Dropdown options={options} selection search />)

// open the dropdown
wrapper.simulate('click')
dropdownMenuIsOpen()

// select the second item in the list
wrapper.find('DropdownItem').at(1).simulate('click')
wrapper.find('DropdownItem').at(1).should.have.prop('active', true)
wrapper.find('DropdownItem').at(1).should.have.prop('selected', true)

wrapper.find('DropdownItem').at(0).should.have.prop('active', false)
wrapper.find('DropdownItem').at(0).should.have.prop('selected', false)

// search for a non-existent item, triggering the not found message
const search = wrapper.find('input.search')
search.simulate('change', { target: { value: '__nonExistingSearchQuery__' } })

// click outside, close the dropdown
domEvent.click(document)
Copy link

Choose a reason for hiding this comment

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

This line isn't working really. I noted that once you added describe.only, the call domEvent.click(document) isn't really working, and would also cause a failure in the test closes on click outside because it uses the same call. If you use document.body.click() (like the test blurs the Dropdown node on close by clicking outside component) or domEvent.click(document.body) then things start working. What this causes though is a call to closeOnDocumentClick, which causes the dropdown menu to close, but the focus is still on the input and the input search text is never cleared. In this case, this is correct behavior, since the menu has been closed, but focus is still on the input, so the invalid search text should not be removed. What the test should be doing is calling the handleBlur function which is what you're trying to test with this test case. In summary,

  1. Your checks below with the wrapper.find() calls aren't working because the dropdown still has the search text in the input and is showing the No results message.
  2. Need a different way to trigger a blur event instead of document.body.click(). This applies to this test case and to the blurs the Dropdown node on close by clicking outside component case. May need to include another button element to throw the focus to cause a blur to happen.
  3. Actually, I believe the dropdown is working well with your changes, its just the test case isn't correctly testing it

dropdownMenuIsClosed()

// next time we open the dropdown, the active item at index 1 should be selected
// even though our search query last set the selected index to 0
wrapper.simulate('click')
dropdownMenuIsOpen()

wrapper.find('DropdownItem').at(1).should.have.prop('active', true)
wrapper.find('DropdownItem').at(1).should.have.prop('selected', true)

wrapper.find('DropdownItem').at(0).should.have.prop('active', false)
wrapper.find('DropdownItem').at(0).should.have.prop('selected', false)
})

it('set right selected index when click on option with search', () => {
wrapperMount(<Dropdown options={options} search selection />)

// avoid testing item index 0, it is selected by default
const itemIndex = 1

// open the menu
wrapper.simulate('click')
dropdownMenuIsOpen()

// search for specific item, filtering the list and changing the index order
const search = wrapper.find('input.search')
search.simulate('change', { target: { value: `${itemIndex}-item` } })

// click the item
wrapper.find('DropdownItem').simulate('click')

// open the menu again
wrapper.simulate('click')
dropdownMenuIsOpen()

// our selectedIndex should match that items index in the full list of items
wrapper.state('selectedIndex').should.equal(itemIndex)
})

it('still allows moving selection after blur/focus', () => {
// open, first item is selected
wrapperMount(<Dropdown options={options} selection search />)
Expand Down