Exposing onChangeStart as a callback#66
Conversation
Includes change to readme + test.
|
|
||
| if (this.props.onChangeStart) { | ||
| this.props.onChangeStart(this.props.value); | ||
| } |
There was a problem hiding this comment.
Could you please add a linebreak between } and the next line? Thanks!
| const onChange = jasmine.createSpy('onChange').and.callFake(value => component.setProps({ value })); | ||
| const onChangeStart = jasmine.createSpy('onChangeStart'); | ||
| const jsx = ( | ||
| <InputRange |
There was a problem hiding this comment.
Could you please fix the indentation of this JSX block? At the moment, I think there are 3 spaces; instead, there should be 6. :)
| onChangeStart={onChangeStart} /> | ||
| ); | ||
| const component = mount(jsx, { attachTo: container }); | ||
| // eslint-disable-next-line quotes |
There was a problem hiding this comment.
Instead of disabling this rule, can you actually fix the ESLint issue? So, just use single quote 'Slider [onMouseDown]'.
There was a problem hiding this comment.
just removed the eslint line, didn't give any issues. I imagine, its just my editor.
| document.dispatchEvent(new MouseEvent('mousemove', { clientX: 100, clientY: 50 })); | ||
| document.dispatchEvent(new MouseEvent('mousemove', { clientX: 150, clientY: 50 })); | ||
| document.dispatchEvent(new MouseEvent('mouseup', { clientX: 150, clientY: 50 })); | ||
| expect(onChangeStart.calls.count()).toEqual(0); |
There was a problem hiding this comment.
I think you want to assert that onChangeStart should be called if it is defined. Currently, it asserts that it shouldn't be called?
copy-pasted from a section above to make sure my editor wasn’t up to some weird tricks :)
sorry about that, was dealing with editor stuff!
|
@davidchin Thanks for the quick comments, and sorry about the annoying indentation changes. I've made the changes requested. But there is one issue, the test is failing now. It seems to work when checked from the example, so I imagine I am setting up the test wrong. I'm not too familiar with this style of testing, so if could take a look there then that would be much appreciated! |
|
Hey @oyeanuj, thanks for doing the changes, much apprecated. I think the test is failing at the moment because it is really failing. If you look at |
- also changed the if statement to a positive statement for readability
|
@davidchin Thank you for spotting that! Added the commit to fix it, and the tests are passing now :) |
2 participants
Hi @davidchin!
I noticed that there was
onChange andonChangeCompleted` but nothing to indicate the start of that action. This is a callback that I needed as I integrated this with a media player. Let me know if this looks good to you!Thank you!