-
Notifications
You must be signed in to change notification settings - Fork 933
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
fix: disabled items should not be selectable(#434) #436
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.
Looking awesome. Just a few things in the tests.
"react-test-renderer": "^16.2.0", | ||
"react-testing-library": "^1.1.0", | ||
"react-test-renderer": "^16.3.2", | ||
"react-testing-library": "^2.3.0", |
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.
Thanks for the update!
renderSpy, | ||
input, | ||
arrowDownInput: extraEventProps => | ||
Simulate.keyDown(input, {key: 'ArrowDown', ...extraEventProps}), |
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.
Since we're rendering into the document, could we use fireEvent
instead?
const firstItem = utils.queryByTestId('item-0') | ||
expect(firstItem.hasAttribute('disabled')).toBe(true) | ||
|
||
// input.simulate('keydown') |
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 comment here doesn't add anything I think. The name of the function is sufficient documentation :)
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.
👍
<div> | ||
<input {...getInputProps({'data-testid': 'input'})} /> |
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.
What's the purpose of this data-testid
?
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.
Later on in this function I query for the input element:
const input = utils.queryByTestId('input')
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.
Ah, I missed that. Thanks!
@@ -514,6 +514,12 @@ class Downshift extends Component { | |||
Enter(event) { | |||
if (this.getState().isOpen) { | |||
event.preventDefault() | |||
const itemIndex = this.getState().highlightedIndex |
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.
These changes are 👍 Thanks!
fireEvent.keyDown(input, {key: 'Enter', ...extraEventProps}), | ||
changeInputValue: (value, extraEventProps) => { | ||
input.value = value | ||
fireEvent.change(input, {...extraEventProps}) |
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.
I change the value of the input manually beforehand. Is there a better way to do it like passing it to the event?
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.
Maybe, but I prefer this anyway 👍 Thanks!
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.
Thank you! This is great 👍
fireEvent.keyDown(input, {key: 'Enter', ...extraEventProps}), | ||
changeInputValue: (value, extraEventProps) => { | ||
input.value = value | ||
fireEvent.change(input, {...extraEventProps}) |
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.
Maybe, but I prefer this anyway 👍 Thanks!
<div> | ||
<input {...getInputProps({'data-testid': 'input'})} /> |
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.
Ah, I missed that. Thanks!
Would you like to add yourself to the contributors table? |
Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the |
…hift-js#436) * disabled items cannot be selected * updated react-test-renderer and react-testing-library * added test for checking selection behaviour of disabled items * removed comment, used fireEvent instead of Simulate
Problem #434
At the moment it is possible to select a disabled item by pressing enter although selection by click is disabled. Personally, I think it should not be possible to select a disabled item (at least not for the user).
Solution
The event handler checks the DOM-node of the item for the disabled prop.