Input WIP

[liamcmitchell]

Work in progress:

• no tests
• haven't looked at inline styles

Comments welcome.

[stevoland]

Great stuff! No comments from me at the moment, it all looks spot on.

[pieterv]

This is looking really good!

A thought i had when looking at this is i wonder if it might also be a good idea for the type prop to allow react components. This could let you use the bootstrap styling while extending the behaviour of the input, for example:

var big$$$Input = <Input type={<FancyAutoGrowTextInput />} addonBefore="$" />

Probably would need to use cloneWithProps to pass down the needed props.

Im happy to do a code review if you feel like the code is in a place where that would be helpful.

[liamcmitchell]

Thanks, I was working on it just now. (I see you're in NZ too, yay for 3 day weekends!)

My idea was to have a 'custom' type and render props.children inside the wrappers. In either case, passing in arbitrary components could break getValue and other features but I think this is really useful in abstracting away bootstrap's form-group, label & input wrapper boilerplate.

In my case I want to render a date range which is two inputs inside an input-group.

<Input type="custom" label="Date:">
  <DateRange ref="date" fromDate="...">
  ...
</Input>

Would using cloneWithProps be safe in this case? What about if there are multiple children?

[liamcmitchell]

Thinking about it more, maybe it would be better renamed as FormGroup. Rendering children is the default but other elements are rendered as a convenience if type, label, wrapper etc are provided.

This would support any of the following uses:

<FormGroup ref="1" type="text" label="Text:" />

<FormGroup label="Text:">
  <input ref="2" />
</FormGroup>

<FormGroup>
  <label>Text:</label>
  <input ref="3" />
</FormGroup>

However, this would make using transferPropsTo on the input, less intuitive, props would have to be transferred to the form-group div. This would be fine for event handlers because they bubble up but more difficult for everything else.

Yeah, better off as input, thats what we really care about.

[pieterv]

3 day weekends rock, i knew the queen was good for something :) You in Auckland?

Would using cloneWithProps be safe in this case? What about if there are multiple children?

You would want to use React.Children.only which will throw if this.props.children is not a single component.

passing in arbitrary components could break getValue

I was thinking about this too, i seems like a good idea to remove the getValue function and grab the value off a passed in onChange prop via event.target.value, this is how ReactLink does it. This would also allow custom components to pass the value directly back from onChange instead of the event.

For example, the value and onChange props would be injected from Input when rendered.

<DateRange fromDate="..." value={value} onChange={handleChange} />

My idea was to have a 'custom' type and render props.children inside the wrappers

Here are some thoughts i had on the different API's:

// 1
<Input type="custom" label="Date:">
  <DateRange ref="date" fromDate="...">
</Input>

// 2
<Input type={<DateRange ref="date" fromDate="...">} label="Date:" />

// 3
<FormGroup>
  <label>Text:</label>
  <DateRange ref="date" fromDate="...">
</FormGroup>

• The type="custom" feels a little redundant since we know its a custom type because of the child passed in.
• Wont be immediately obvious what is acceptable to be passed as children, can multiple components be passed? etc
• Easier to express the custom input component, especially if it requires a lot of config.

• Passing components as props can be a little clunky as you can sometimes get component inception where they nest a few levels deep, where this feels pretty natural when using props.children.
• I feel like this reads kinda nice, like input type is DateRange. (This is likely my bias since i suggested it :P)

• I feels like this API would be too open, with a number of ways to express different component states, in my experience this can be confusing as user.
• Sounds pretty hard to implement, as you would need to parse the children and pick out the different components, which might not always be easy to tell what goes where :P

[liamcmitchell]

Yes, I'm in Auckland.

I agree type="custom" is redundant, it should be understood if no type is specified and children are passed in.

I don't think manipulating children is good practice. In the date range case, there are actually two values (from & to). Trying to get Input to handle this through getValue doesn't sound like fun.

Instead I think components should be managed wherever they are created. If you want to use a special component inside the Input wrappers, you should be entirely responsible for it.

Because Input will use transferPropsTo on it's internal input/select/textarea, it should cover 90% of use cases without needing to pass in components. In the edge cases, Input should allow use as wrapper.

// 90% use case
<Input type="text" label="Text:" value={...} onChange={...} />

// Special case, Input used as wrapper only
<Input label="Text:">
  <input value={...} onChange={...} />
  <input value={...} onChange={...} />
  ...
</Input>

// Which could be wrapped as a separate component if reused
<SpecialInput label="Text:" value1={...} value2={...} onChange={...} />

Could your FancyAutoGrowTextInput case work this way?

[pieterv]

Yeah that makes sense, would definitely work for the use cases i was thinking of.

I see what you were saying earlier about the Input component name not being quite right especially when used as a wrapper. I wonder if there would be value in splitting the two use cases out into separate components.

// 90% use case
<Input type="text" label="Text:" value={...} onChange={...} />

// Special case, used as wrapper only
<FormGroup label="Text:">
  <input value={...} onChange={...} />
  <input value={...} onChange={...} />
  ...
</FormGroup>

The two use cases actually have very different API's, and this might help make it clearer, Input requires a type, a value and an onChange callback. Whereas FormGroup (or insert better name here) can just take a label, etc. Internally you could use the FormGroup component inside the Input.

Just a thought otherwise the previous API you stated sounds good.

[liamcmitchell]

This is the current API:

<Input
  type="text" // Any HTML input type as well as select, textarea & static.
  label="Text:" // Rendered in label element.
  help="Enter text." // Rendered in .help-block span.
  addonBefore="$" // Rendered in input-group.
  addonAfter=".00" // Rendered in input-group.
  bsStyle="warning" // One of success, warning or error. Adds validation classes.
  hasFeedback={true} // Renders validation icon.
  groupClassName="group"
  wrapperClassName="wrapper"
  labelClassName="label"
  // All properties are applied to the final input/select/textarea/p element using transferPropsTo.
/>

// Children are rendered for type=select...
<Input type="select">
  <option value="1">
  <option value="2">
</Input>

// or if no type specified (used as a wrapper only).
<Input label="..." wrapperClassName="...">
  <img src="...">
</Input>

[liamcmitchell]

I don't think it's worth splitting out because checkbox & radio require different layouts, meaning only the outer form-group div can be separated.

What about if we did it the other way around. FormGroup is a thin wrapper around Input that renders Input as a wrapper by forcing type to null? That would give FormGroup all the same wrappers but without getValue or the input element.

Select and TextArea could also be done the same way but I don't know if it's that helpful.

[pieterv]

This is looking really good.

I see what you mean about the FormGroup not being worth splitting out. I guess we go with how you have it without the wrappers, we can see how the API feels and add them in if needed :)

[stevoland]

Yeah, sorry for the lack of input from me. Looks great guys.

[liamcmitchell]

I'm happy with this. Do you want me to rebase/squash the commits?

[sophiebits]

This isn't currently supported; it should be this.refs.input.getDOMNode().value.

[sophiebits]

Er, never mind me.

[liamcmitchell]

This uses the new Input component which has a getValue method, not the React input component.

[sophiebits]

Yeah, sorry about that. :) (It's actually likely that React's input will support getValue too but it doesn't today.)

[liamcmitchell]

No worries :)

[liamcmitchell]

Added getChecked and getInputDOMNode.

Checked is handled differently to value but we could support both as a convenience, like I did with the static type. Is it worth the effort? Are there any disadvantages?

[pieterv]

I think what you have looks really good, with how it is currently you still have the option to pull the value out of the field if you need it.

Squashing the commits would be good, then we can get @stevoland to merge :)

[stevoland]

This is so great, thanks!

Just one style point, could you replace the == with ===.
If you could also rebase with master and squash the commits into one I will get it merged.

Thanks again!

[liamcmitchell]

@stevoland Done

[stevoland]

@liamcmitchell Awesome, thanks.