[Slider] Normalize onChange event parameters

[jonrhall]

• This is a v1.x issue (v0.x is no longer maintained).
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

Currently, the onChange event parameters on the Slider component are (event, value). While this works and is fine in and of itself, it is not consistent with the function parameters for other onChange events within the core Material UI library. For example, the v1 TextField component has onChange parameter of (event) with the value of the event itself being bound in the nested property event.target.value, as does the Select component.

The latter way of doing things seems to be "more correct". Doing it that way would help developers like myself generalize input behavior across different components (including Slider) that are used side-by-side.

Current Behavior

Currently, the onChange event parameters on the Slider component are (event, value).

Steps to Reproduce (for bugs)

Just go to the first example on the Material UI website: https://material-ui.com/demos/selects/#simple-select

Context

I'm trying to tie a TextField component to a Slider component so that the user has a choice over what input type to use. I have a general handleInput method which takes event.target.value and assigns it to the state of my React component, which I would also like to use with the Select component but can't without first wrapping the onChange event like so:

onChange={(event, val) => this.handleInput({ event: { target: { value: val } } })}

Your Environment

Tech	Version
Material-UI	v1.1.0
React	v16.4.0
material-ui/lab	v1.0.0-alpha.5

[oliviertassinari]

@jonrhall Interesting issue, I haven't looked at the Slider implementation, so I can't tell you if it's a good or a bad idea. Theoretically speaking if the event comes from a click, I don't think that we can move forward with this proposition. If the event is backed by a native slider component, it could work out of the box.
Also, It's something to benchmark with the other slider implementation available in the wild.

[sakulstra]

I had a quick look and it seems like material-ui is not using the native input type="range" under the hood, alternatives like rc-slider don't use it as well. Actually i didn't find an implementation which uses a native range input, so there might be a good reason to it.

[oliviertassinari]

@sakulstra Then I guess we can label this issue "won't fix".

[sakulstra]

should have looked a bit deeper - the libraries i checked support ie9+ (which doesn't support type="range") and have some more advanced range features(which will probably never be supported by material-ui itself anyways). So it might indeed make sense to use a range input under the hood.

I actually just found a library using the range input: https://github.com/OnsenUI/OnsenUI/blob/master/core/src/elements/ons-range.js#L95 (never heard of it before though)

[jonrhall]

I'm unfamiliar with the general course of action within the Material UI community about these types of issues... Are you both saying that Material UI isn't in the business of being opinionated about what types of events are emitted from the Material UI components that act as inputs, and that their handlers essentially act as passthroughs for native input elements? I just want to make sure I'm reading you correctly.

If that's the case, then yes I would be a proponent of using the native range input if it makes sense, which will probably solve a whole class of issues of using this component alongside more traditional inputs in the future.

[oliviertassinari]

handlers essentially act as passthroughs for native input elements?

@jonrhall Yes, it's what we have been trying to implement so far with all the components!

[uwap]

Additionally currently there seems to be no way to get the value from onDragEnd and onDragStart events. Saving the value from onChangeState to inject it to onDragEnd feels redundant. If the onChangeState will be changed, those maybe should be changed as well.