Fluentui: Input component issues

[kmees]

There are some issues with the input components which make them awkward to use in my opinion, especially when used in combination with form libraries like redux-form or react-forms.

In my opinion, the components should be as dump as possible and the api/functionality of the input controls should a superset of the native<input />, <textarea /> and <select /> controls.

Checkbox

• Component should be named ChoiceField
• Why use isChecked / onChanged as props instead of using the standard checked / onChange
• name prop is missing
• text prop should be names label (to be consistent with Toggle)
• Internal state should be removed. Could even be a functional component by making name or id required.

• defaultChecked and checked() getter should be added (to be consistent with Toggle)

  Toggle

• Why use checked / onChanged as props instead of using the standard checked / onChange

• name prop is missing

• Internal state should be removed. Could even be a functional component by making name or id required. checked() getter should use the DOM element instead of state.

  TextField

• Currently, when neither value nor defaultValue is passed as a prop, the underlying <input /> element switches from uncontrolled to controlled and an error is thrown. This is again an issues when maintaining internal state.

• The prop names for validation callbacks are confusing.
• The validation stuff should be moved into a HOC, i.e. ValidatingTextField, the TextField component does too much stuff in my opinion.
• Should support the value / onChange props standard.

Haven't looked at Dropdown/ChoiceFieldGroup yet.

[dzearing]

Thanks for the feedback, it's helpful to get another pair of eyes on this. I agree with pretty much all of your feedback. A couple of things here:

Regarding "as dumb as possible" 100% yes. We are learning this with many of the controls to extract out dumber sub components; there are a lot of changes coming to reduce/refactor some of these to make cleaner implementations. In some cases we can get away with stateless if we can have React's underlying DOM abstraction do it for us, but not always.

First we are actively cleaning up props that feel "different" from the default HTML attributes without a good reason. Checkbox is definitely an example that hasn't been updated to this standard yet. Originally, Toggle had an isToggled property that we renamed to "checked" to be consistent with the default input HTML element. Toggle has been scrubbed but there are still a few left overs to make consistent; we'll do it.

For background: we have a general javascript coding convention to use the "is, are, should" qualifier prefix for boolean flags, but HTML standards conflict here which is to not use qualifiers. We also have a general rule to use positive variable names like "isVisible" instead of negative "isHidden", but this again conflicts with HTML which uses "disabled" and "hidden" flags. So we've decided that for flags, we will try to follow the HTML convention as much as possible. Not everything has been updated, so if you see contract changes and want to submit an issue, or even better a PR, we'll try to get that patched up.

The onChange vs onChanged naming convention was intentional, but we're open to making it more consistently use onChange. The issue is that in these onChanged callbacks, we're passing back additional metadata to the caller. Toggle for example passes back the checked state. TextField may pass in the value. We don't want the caller to assume ev.target.value is the way to read the value. So by changing the definition we may be changing the expectations. E.g. a user expects onChange to take in a React.InputEvent , but now the event is the 3rd argument. But... well, there is also ambiguity here with onChange vs onChanged so I can see it being confusing either way.

I will look into the controlled/uncontrolled switching. We have a separate issue logged to address and will see if I can scrub these in that process.

Factoring out the validation from text field has definitely been discussed already and agreed on. We should open a separate issue to track this.

[kmees]

Thanks for the response. Good to hear that you are dropping the naming/coding conventions in favour of HTML conventions 👍.

Regarding the components, you can have look at my components (https://github.com/react-fabric/react-fabric/tree/development/src), they're all completely dump and rely only on props (expect SearchBox because the css needs a hover state). I'm also thinking about deprecating the project and start contributing here since they basically accomplish the same thing.

For the onChanged callback, how about renaming it to onValueChange: (value:any) => void and supporting the original onChange: (e:React.InputEvent) => void.

[maxali]

As @kmees mentioned, the standard onChange: (e:React.InputEvent) => void callback implementation is important. There is no way we can reference the changed element without event: React.InputEvent (or let me know if there is a way).

[gabrielruss]

I am currently trying to implement a TextField and I can't figure out how to reference the current element when updating the appropriate values in the state. The old way, like mentioned above, was to use onChange and event.target.name. What is the current workaround for this?

Edit: Thanks to kmees for the workaround.

Create a OnChangeHandler for the TextField component. Like so...

const createOnChangedHandler = (name) => (value) => { /* do something with name and value here, i.e. */ }

Then call the onChangedHandler from your onChanged event.

[notgiorgi]

@gabrielruss did you find a way to do it?

[gabrielruss]

@notgiorgi Yes, I am currently using the method I described in my previous comment. I believe you could also create a parent component for the TextField (or any component type) and specify a name property on the parent component. Then on the child component's onChanged method, call this.props.onChanged and pass this.props.name. This is a strange method...but it works currently.

Here is the onChanged method I have for my child component.

private _handleChange (value: IChoiceGroupOption) { this.props.onChanged(this.props.name, value.text); }

[notgiorgi]

@gabrielruss It got fixed #809

[gitjain]

It got fixed in Jan so closing it.

[brendenseidel]

The above workaround does the job. This is a littler clearer:

handleInputChange = name => value => {
  this.setState({
    [name]: value
  });
}

---

<TextField
  label='Last Name'
  value={this.state.lastName}
  onChanged={this.handleInputChange('lastName')}
/>