-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Select first element matching pressed key in dropdown (#1156) #1160
Select first element matching pressed key in dropdown (#1156) #1160
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things
src/Dropdown.js
Outdated
@@ -132,23 +132,32 @@ class Dropdown extends React.Component { | |||
const disabledClass = mapToCssModules('disabled', this.props.cssModule); | |||
|
|||
const items = container.querySelectorAll(`.${menuClass} .${itemClass}:not(.${disabledClass})`); | |||
const char = String.fromCharCode(e.which).toLowerCase(); | |||
const itemsOfChar = char !== '' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array.from
is is not supported in IE and also does not get transpiled. It would require a polyfill.
src/Dropdown.js
Outdated
@@ -132,23 +132,32 @@ class Dropdown extends React.Component { | |||
const disabledClass = mapToCssModules('disabled', this.props.cssModule); | |||
|
|||
const items = container.querySelectorAll(`.${menuClass} .${itemClass}:not(.${disabledClass})`); | |||
const char = String.fromCharCode(e.which).toLowerCase(); | |||
const itemsOfChar = char !== '' && | |||
Array.from(items).filter(item => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
charAt or direct index access would have better performance
src/Dropdown.js
Outdated
if (items[i] === e.target) { | ||
index = i; | ||
break; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If char
is an empty string itemsOfChar
will be false
and this will throw an error.
src/__tests__/Dropdown.spec.js
Outdated
@@ -452,7 +452,7 @@ describe('Dropdown', () => { | |||
|
|||
expect(Dropdown.prototype.toggle.mock.calls.length).toBe(0); | |||
|
|||
wrapper.find('#first').hostNodes().simulate('keydown', { which: 52 }); | |||
wrapper.find('#dropdown').hostNodes().simulate('keydown', { which: 52 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't work when we trigger keydown
events directly on the dropdown items, but I'm unable to actually replicate that scenario from my browser. It appears to me like this test is a more accurate representation of a user interaction. And it makes the tests pass. Of course, I could be wrong! 😄
#1156