-
Notifications
You must be signed in to change notification settings - Fork 233
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
Exposing onChangeStart as a callback #66
Conversation
Includes change to readme + test.
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.
Hey @oyeanuj, thanks for your contribution. I think it's a good addition. Can you please have a look at my comments? They should be minor changes :)
src/js/input-range/input-range.jsx
Outdated
@@ -414,6 +416,9 @@ export default class InputRange extends React.Component { | |||
return; | |||
} | |||
|
|||
if (this.props.onChangeStart) { | |||
this.props.onChangeStart(this.props.value); | |||
} |
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.
Could you please add a linebreak between }
and the next line? 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.
done!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please fix the indentation of this JSX block? At the moment, I think there are 3 spaces; instead, there should be 6. :)
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.
done!
onChangeStart={onChangeStart} /> | ||
); | ||
const component = mount(jsx, { attachTo: container }); | ||
// eslint-disable-next-line quotes |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) |
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!
Hi @davidchin!
I noticed that there was
onChange and
onChangeCompleted` 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!