Skip to content

Conversation

@astalwick
Copy link

I'm not sure if this is something you'll want to merge in generally, but:

I needed the ability to distinguish between changes that are part of a currently-in-progress drag of the range slider, and changes that are not. In my particular case, I needed it to be able to add (reliable) undo support that does not create a new undo event for every mousemove change, but there are other use cases - eg, knowing when to consider a change done and send results to a server.

I've added an onInteractiveUpdate callback. If it is supplied, the mousemove/touchmove changes are called back there. On mouseup/touchup, a final onChange call is made.

If onInteractiveUpdate is not supplied, everything behaves as normal (onChange is called for mousemove/touchmove, as before).

… slider changes (mousemove/touchmove) and final slider value change (mouseup/touchup).
@davidchin
Copy link
Owner

Thanks @astalwick! I understand your use case and your need for this change. However, I think it's kinda confusing, as adding onInteractiveUpdate prop alters the behaviour of onChange callback. Also, to me, all user-triggered events (mouse/keyboard) should be considered as 'interactive'.

I'm not sure how you implement your undo function. I guess you have an undo stack, and in your onChange callback, you push a new command to the stack so it could be repeated when you call undo. I thought you might be able to get around your issue by using a debounce function in your app code; so you only push to the undo stack every 500ms (or longer) instead of every mouse change. For example:

const pushToUndoStack = _.debounce((value, oldValue) => {
  // Create a new command
}, 500);

const onChange = (component, value) => {
  pushToUndoStack(value, this.state.value);
  this.setState({ value });
};

Anyway, just an idea, as I haven't tested it, what do you think?

@astalwick
Copy link
Author

Yeah, debounce solves it for the 95% case, and initially, that was what I was going to do. Unfortunately, it leaves some edge cases: user drags, hesitates, undo is registered, continues dragging, lets go, undo is registered. Two undo events for one action. Acceptable, but not ideal. Especially when you're sitting there going "but, the mouse up is in there, I just need to be told about it!"

Anyway, I understand your hesitation to pull this in - it's a bit awkward. I hesitated to submit it as a pull request. I wrote the code trying to keep onChange backwards compatible (in fact, onChange behaves the same as before unless you've set the onInteractiveUpdate handler -- so this shouldn't be a breaking change for anyone currently using it). Still, though, I don't like the name (onInteractiveUpdate), and the code is not as general as I would like.

So, instead, how about this:
onChange remains the same, but simply includes the event, e, as an easily ignorable third argument. onChange(component, value, e). In client code, if you care about the difference between mouseUp and mouseMove, you just inspect the source event yourself. The benefit, too, is that this would apply to any events. I could see the same argument being made for a user holding down the an arrow key: repeated onChange events, followed by a final change event when the arrow key is released. Only the final change should be undoable (or posted to the server, or acted upon in the browser if you're doing very computationally heavy updates).

@davidchin
Copy link
Owner

@astalwick good suggestion. I thought about the problem for a bit and came up with a solution. You could have a look at this PR (#10). I think it'll solve your problem. Basically I added a callback onChangeComplete, which gets called once when you finish dragging a slider. For example:

<InputRange
  maxValue={20}
  minValue={0}
  value={this.state.value}
  onChange={this.handleValueChange.bind(this)}
  onChangeComplete={this.handleValueChangeComplete.bind(this)}
/>

So onChange is still needed, as it is responsible for updating your model value. On the other hand, onChangeComplete informs you when an interaction completes (mouseup/keyup). Therefore, you can use it to handle undo logic (or posting to server).

@astalwick astalwick closed this Dec 18, 2015
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.

2 participants