Skip to content

Conversation

@davidchin
Copy link
Owner

Few changes here:

  1. Modified onChange callback parameters:
    from onChange(component, value) to onChange(event, value, component)
  2. Added onChangeComplete callback. It gets called once after completing a change. For example, if you drag a slider and change the value from 5 to 10, onChangeComplete gets called once when you release your drag (on the other hand, onChange callback gets called 5 times during the course of the drag and is responsible for updating model value).
<InputRange
  maxValue={20}
  minValue={0}
  value={this.state.value}
  onChange={this.handleValueChange.bind(this)}
  onChangeComplete={this.handleValueChangeComplete.bind(this)}
/>

@astalwick
Copy link

This is great; much cleaner than my PR, and solves my problem nicely. The only comment I have is that maybe the order of the arguments is a bit weird.

Why not keep onChange's argument order the same, with event as the third argument?

By changing the order and putting event first, you've made this a breaking change for people using 0.5.0 (and it doesn't really need to be).

@davidchin
Copy link
Owner Author

Thanks for your input @astalwick. Regarding the param ordering, I just thought it'd be good to keep the interface consistent with other event handlers in React. (for example: <input type="text" value={this.state.value} onChange={this.handleChange}>). Not entirely necessary though... But it seems like it is the convention (maybe?) when you set up an event-driven callback in a custom component? Anyway, I agree, the down-side is that it'd be a breaking change. But I'm okay with that. What's your thought?

@davidchin
Copy link
Owner Author

Ok, I took out the changes to onChange for now.

davidchin added a commit that referenced this pull request Dec 23, 2015
Notify delegate when interaction is complete
@davidchin davidchin merged commit 8935903 into master Dec 23, 2015
@davidchin davidchin deleted the event_type branch January 15, 2016 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants