[Slider] Pass this.state.value into onDragStop & onDragStart

[Ehesp]

• PR has tests / docs demo, and is linted.
• Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
• Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Someone I know was having issues getting the slider value back when the user stops sliding. It was being passed back whilst sliding (onChange) but that's now what they were after.

From what I can see, the only way of getting that value is creating a ref on the Slider component and accessing this.refs.slider.getValue() within the onDragStop method. Like the onChange method, the second param is now the slider value. I also added the same to onDragStart for consistency.

[oliviertassinari]

@Ehesp I don't get it. What the issue with onChange in the first place? I would rather not increase the API surface if not necessary.
Providing a second parameter doesn't look like something standard.

From what I can see, the only way of getting that value is creating a ref on the Slider component and accessing this.refs.slider.getValue()

Well, most component method should probably be considered internal.
I think that imperative method like getValue should be deprecated.

[oliviertassinari]

@Ehesp I'm gonna close this PR. We forward the onDragStart so users can still have access to native DOM event, not to hack around. I think that it would be better to fix the core issue. Thanks anyway!

[karlfloersch]

I believe there is a valid use case where you cannot use onChange but you still need to access this.state.value. It is as follows:

The slider.value is mapped to global state which is constantly incrementing. You would like to set that global state onDragEnd which would then change the slider position.

You can't use onPosChange because you need to update the global state, and updating it in onPosChange would be a loop.

This problem is clear if you want to use the slider for a media player. You want to keep the progress bar in sync with the music, and then update the music position when the onDragEnd is triggered.

What do you think?

[jdelafon]

I also have a use case where I want stuff happening when the slider moves (e.g. changing grey scale in a canvas), while some DOM change should only happen on release, because otherwise the slider moves too fast for the action to complete (in this case, resizing a second slider). In other words, any event callback that takes more time to complete than for the slider to move by 1 tick cannot be triggered onChange.

Having the value available onDragEnd would allow for instance to construct a range slider (with two handles) made of two simple sliders easily. The size of the second needs to change only when the first is released.

I don't understand what the issue is to passing the value just like onChange does. The event object is not enough since event.target.value is undefined as well.

To complete the OP comment, a ref on the slider will not have the current slider value but only that of the previous tick.

[oliviertassinari]

You can't use onPosChange because you need to update the global state, and updating it in onPosChange would be a loop.

@karlfloersch I was thinking about exposing a callback property that is called when the slider position drag is over. We could call it onChangeEnd. Or do you have another idea of API that we could expose to solve the issue (without going against the DOM specification)?

In other words, any event callback that takes more time to complete than for the slider to move by 1 tick cannot be triggered onChange.

@jdelafon Sounds like an issue on userland. For this use cases, why not throttling the callback?

Actually, I feel like you are having issues with handling the asynchronous aspect of the <Slider />.
I pretty sure those issues could be solved on userland taking advantage of the exposed property and RxJs (bindCallback, throttle and withLatestFrom).

[jdelafon]

Because firstly, you want the onChange event to really fire on every tick, and secondly throttle will silence the observable for a duration, which means that you could (will most likely) miss the only tick that is meaningful for the onDragStop event: the last one.

But I am not really familiar with this, and I am not saying there is not some way to do it somehow. I've always had the onMouseUp-like event I needed until now. Anyway, if you plan adding an onChangeEnd event, that is all good.

[karlfloersch]

@oliviertassinari Adding onChangeEnd sounds good. Though curious, is the reason you don't want to add the slider.value to onDragStop because onDragStop is a common web API? Basically, you don't want to confuse people by providing a non-standard call?

Just to verify, onChangeEnd get called one time when you stop dragging the slider, correct? And is it true that every time onDragStop is called, onChangeEnd will be called, and visa versa?

[oliviertassinari]

Basically, you don't want to confuse people by providing a non-standard call?

Yes indeed.

I'm gonna submit a PR with the change. Would be awesome if you can have a look once it's done.
Having a look at some other popular <Slider /> component:

• https://github.com/mpowaga/react-slider
• https://github.com/davidchin/react-input-range
• https://github.com/react-component/slider

It looks like onAfterChange is the most common used name for this behavior.

[karlfloersch]

@oliviertassinari I'd be happy to review the PR! 😄 using onAfterChange then makes sense to me.

[oliviertassinari]

I have noticed one use case that you might not be able to handle even with the new callback.
What about a user using the keyboard to change the value (repetitive key strokes)?

[madsonic]

doesn't seem like onAfterChange or onChangeEnd was added?