Skip to content
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

Merged
merged 7 commits into from Mar 10, 2017

Conversation

Projects
None yet
2 participants
@oyeanuj
Copy link
Contributor

oyeanuj commented Mar 9, 2017

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!

Exposing onChangeStart as a callback
Includes change to readme + test.
@davidchin
Copy link
Owner

davidchin left a comment

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 :)

@@ -414,6 +416,9 @@ export default class InputRange extends React.Component {
return;
}

if (this.props.onChangeStart) {
this.props.onChangeStart(this.props.value);
}

This comment has been minimized.

Copy link
@davidchin

davidchin Mar 9, 2017

Owner

Could you please add a linebreak between } and the next line? Thanks!

This comment has been minimized.

Copy link
@oyeanuj

oyeanuj Mar 9, 2017

Author Contributor

done!

const onChange = jasmine.createSpy('onChange').and.callFake(value => component.setProps({ value }));
const onChangeStart = jasmine.createSpy('onChangeStart');
const jsx = (
<InputRange

This comment has been minimized.

Copy link
@davidchin

davidchin Mar 9, 2017

Owner

Could you please fix the indentation of this JSX block? At the moment, I think there are 3 spaces; instead, there should be 6. :)

This comment has been minimized.

Copy link
@oyeanuj

oyeanuj Mar 9, 2017

Author Contributor

done!

onChangeStart={onChangeStart} />
);
const component = mount(jsx, { attachTo: container });
// eslint-disable-next-line quotes

This comment has been minimized.

Copy link
@davidchin

davidchin Mar 9, 2017

Owner

Instead of disabling this rule, can you actually fix the ESLint issue? So, just use single quote 'Slider [onMouseDown]'.

This comment has been minimized.

Copy link
@oyeanuj

oyeanuj Mar 9, 2017

Author Contributor

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);

This comment has been minimized.

Copy link
@davidchin

davidchin Mar 9, 2017

Owner

I think you want to assert that onChangeStart should be called if it is defined. Currently, it asserts that it shouldn't be called?

oyeanuj added some commits Mar 9, 2017

fix indentation
copy-pasted from a section above to make sure my editor wasn’t up to
some weird tricks :)
removed extra eslint disable
sorry about that, was dealing with editor stuff!
@oyeanuj

This comment has been minimized.

Copy link
Contributor Author

oyeanuj commented Mar 9, 2017

@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!

@davidchin

This comment has been minimized.

Copy link
Owner

davidchin commented Mar 10, 2017

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 handleInteractionStart method, you can see there's a guard statement - return early if onChangeComplete is undefined. That explains why it works in the example, but not in your test. So what you want instead is to move this.props.onChangeStart(this.props.value) above that guard statement. That will fix your issue.

Moved the order of statements to prevent early return
- also changed the if statement to a positive statement for readability
@oyeanuj

This comment has been minimized.

Copy link
Contributor Author

oyeanuj commented Mar 10, 2017

@davidchin Thank you for spotting that! Added the commit to fix it, and the tests are passing now :)

@davidchin
Copy link
Owner

davidchin left a comment

Thank you!

@davidchin davidchin merged commit 4ca6ea2 into davidchin:master Mar 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@oyeanuj

This comment has been minimized.

Copy link
Contributor Author

oyeanuj commented Mar 11, 2017

Awesome, thanks for merging! Also, do take a look at PRs #68 and #69 whenever you get a moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.