Skip to content
This repository has been archived by the owner on Jul 19, 2019. It is now read-only.

[fixed] #309 - autoHighlight not working unless match is beginning of… #310

Closed
wants to merge 1 commit into from

Conversation

epegzz
Copy link

@epegzz epegzz commented Jan 31, 2018

See #309

The current implementation only auto-highlights an item if the input matches the beginning of the item label.

That's problematic if your shouldItemRender implementation allows matching anywhere in the item label, not just from the start.

This PR fixes that.

@@ -65,7 +65,7 @@ describe('Autocomplete acceptance tests', () => {
it('should highlight top match when `props.value` changes', () => {
const tree = mount(AutocompleteComponentJSX({}))
expect(tree.state('highlightedIndex')).toEqual(null)
tree.setProps({ value: 'a' })
tree.setProps({ value: 'ri' })
Copy link
Author

Choose a reason for hiding this comment

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

The item we are matching here is Arizona.
In the old test we typed a, which put the Arizona item first and auto-highlighted it.

But if you typed ri then still Arizona was put as first item, but it was not auto-highlighted, since ri does not match the beginning of Arizona.

With the changes in this PR, Arizona will now correctly be highlighted.

@@ -375,7 +375,7 @@ class Autocomplete extends React.Component {
const itemValue = getItemValue(matchedItem)
const itemValueDoesMatch = (itemValue.toLowerCase().indexOf(
value.toLowerCase()
) === 0)
) > -1)
Copy link
Author

@epegzz epegzz Jan 31, 2018

Choose a reason for hiding this comment

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

I'd argue, that itemValueDoesMatch is even unnecessary.
If an item is displayed because it passed the check of shouldItemRender then we can also auto-highlight it if it's the first item in the list.

@CMTegner
Copy link
Collaborator

CMTegner commented Feb 4, 2018

The existing logic won't be changed at the moment. See #239 for explanation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants