Fix: Allow user to provide input props

[joshq00]

Fixes #82

• Update change events use event instead of value.
• Props solely used for input components, where able, are directly passed

There are still some components where value, id, name are handled manually.

[tleunen]

Thanks @joshq00! I really appreciate the help to make react-mdl better :)

I understand we could do a cleanup in the propTypes of every component, but is some cases and especially when they are flagged "required", I think it's good to have them. It forces the user to use/set them and to know what's available on the component.

[joshq00]

I don't see any problem with keeping them in the propTypes, or documenting that all input attributes are valid props.

However, I don't think they should be required if they are not required on the native input element.

For example, if I want a text box without a label (maybe I'm just using placeholder, or putting my label elsewhere) - I have to do

<Textfield label="" />

Without doing label="", I don't just get an invariant warning... I actually get an error thrown

[tleunen]

I think it really depends. Because you know how the component is built, you know you can use a lot of props that are not described. But ideally, props should describe what you can use (or at least those who are supported and tested). A slider without the min/max value for example doesn't make a lot of sense, but because it's an input behind the scene, you know you can provide them.

In the implementation of a MDL Textfield, the label is required (for the bottom border when it's focused). So that's the reason why it's needed.

[joshq00]

I made a fiddle testing behavior with/without labels

Everything appears to be working properly without labels

[tleunen]

Yes the input still works. But it doesn't have the MDL styles. Label is required with a MDL Textfield, we cannot change that.

[joshq00]

Fair enough.

Anyhow, I didn't remove isRequired from the label property. Would you like me to add other properties?

[tleunen]

For now, because there's no documentation, I prefer keeping the propTypes because they are kind of a documentation as well.

Could you rebase instead of merge? To avoid having these merge commits ? :)
Thanks!

[joshq00]

Rebased

[tleunen]

Could you please only change the way the event is sent inside onChange?
As I said, I prefer keeping the props to serve as a documentation.

[joshq00]

I'll get those back in today or tomorrow at latest

[joshq00]

@tleunen should be all ready now

[tleunen]

any reason why it's not required anymore?
Usually, React wants us to provide an onChange callback if we set a value.
This is valable for every input.

[joshq00]

Exactly my reasoning. React already handles the checking and it's not required on the true input element. Warnings will show as they should, and the user will interact with the element like they would a normal input element.

As a user, I might not care to listen to the onChange event. I may be prototyping.

Especially in the case of the other input types, I often use defaultValue instead of value+onChange

Making the fewest assumptions about what the user "meant to do" keeps the library from limiting what they can do

[tleunen]

Yeah the only reason why I see onChange not beeing required is just for prototyping because otherwise you have no way to get the value from the input.

[joshq00]

Or, one could have a dom listener registered

[tleunen]

seriously?! Either you use react, or you don't...

I understand it's nice to be generic, but I think there's a limit to that lol

[joshq00]

Not exactly. Take Facebook for example. The majority of the site was done directly mutating dom and they implemented react just for the chat section. There's a reason Facebook doesn't make assumptions about what we meant to do. I can have a full blown site that I just drop in a React component for one piece. That shouldn't break the rest of my app.

[joshq00]

Another example is if the user is looking at onClick instead of onChange. Or onTouchStart/onTouchEnd

Assumptions make an opinionated framework and, if I wanted that, I'd use ember or angular

[tleunen]

In general, yes we can set the onChange callback as optional. It helps prototyping screen. But usually people will set it anyway because we usually create controlled form. (using value + onChange).

[joshq00]

100% agree - you'll probably will want to register a handler... but you might not. there are a million edge cases where you could be accomplishing the same thing.

A use case for not having value is if I'm using a store to keep track of the freeform text instead of state.
Another use case is that, sometimes, I need to use onKeyUp instead of onChange. By requiring it, I need to do onChange={ () => {} } just to avoid all the warnings

[faergeek]

@tleunen Take a look at this about onChange #55

[tleunen]

Yep I forgot about this valueLink. But I agree with the changes. I'll merge once I get a confirmation for the tabindex thing. I asked them on their gitter channel.