Skip to content

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

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

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 all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 81 additions & 3 deletions src/modules/Dropdown/Dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,20 @@ export default class Dropdown extends Component {
debug('dropdown closed')
}

// TODO: Verify this piece
// // 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()
// }

if (prevState.selectedIndex !== this.state.selectedIndex) {
this.scrollSelectedItemIntoView()
}
Expand Down Expand Up @@ -378,8 +392,6 @@ export default class Dropdown extends Component {
debug('closeOnDocumentClick()')
debug(e)

if (!this.props.closeOnBlur) return

// If event happened in the dropdown, ignore it
if (this.ref.current && doesNodeContainClick(this.ref.current, e)) return

Expand Down Expand Up @@ -572,6 +584,71 @@ export default class Dropdown extends Component {
search: this.props.search,
})

// TODO: verify this piece
// let filteredOptions = options
//
// // filter out active options
// if (multiple) {
// filteredOptions = _.filter(filteredOptions, (opt) => !_.includes(value, opt.value))
// }
//
// // filter by search query
// if (search && searchQuery) {
// if (_.isFunction(search)) {
// // 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)
// }
// } else {
// // remove diacritics on search input and options, if deburr prop is set
// const strippedQuery = deburr ? _.deburr(searchQuery) : searchQuery
//
// const re = new RegExp(_.escapeRegExp(strippedQuery), 'i')
//
// filteredOptions = _.filter(filteredOptions, (opt) =>
// re.test(deburr ? _.deburr(opt.text) : opt.text),
// )
// }
// }
//
// // insert the "add" item
// if (
// allowAdditions &&
// search &&
// searchQuery &&
// !_.some(filteredOptions, { text: searchQuery })
// ) {
// const additionLabelElement = React.isValidElement(additionLabel)
// ? React.cloneElement(additionLabel, { key: 'addition-label' })
// : additionLabel || ''
//
// const addItem = {
// key: 'addition',
// // by using an array, we can pass multiple elements, but when doing so
// // we must specify a `key` for React to know which one is which
// text: [additionLabelElement, <b key='addition-query'>{searchQuery}</b>],
// value: searchQuery,
// className: 'addition',
// 'data-additional': true,
// }
// if (additionPosition === 'top') filteredOptions.unshift(addItem)
// else filteredOptions.push(addItem)
// }
//
// this.getMenuOptions.lastReturn = filteredOptions
//
// return filteredOptions

return _.get(options, `[${selectedIndex}]`)
}

Expand Down Expand Up @@ -818,7 +895,8 @@ export default class Dropdown extends Component {
if (triggerSetState) {
this.setState({ open: true })
}
this.scrollSelectedItemIntoView()
// Proposed to remove
// this.scrollSelectedItemIntoView()
}

close = (e, callback = this.handleClose) => {
Expand Down
62 changes: 61 additions & 1 deletion test/specs/modules/Dropdown/Dropdown-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ const dropdownMenuIsOpen = () => {

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

describe('Dropdown', () => {
describe.only('Dropdown', () => {
beforeEach(() => {
attachTo = undefined
wrapper = undefined
Expand Down Expand Up @@ -2162,6 +2162,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