{"action":"created","issue":{"url":"https://api.github.com/repos/react-toolbox/react-toolbox/issues/652","repository_url":"https://api.github.com/repos/react-toolbox/react-toolbox","labels_url":"https://api.github.com/repos/react-toolbox/react-toolbox/issues/652/labels{/name}","comments_url":"https://api.github.com/repos/react-toolbox/react-toolbox/issues/652/comments","events_url":"https://api.github.com/repos/react-toolbox/react-toolbox/issues/652/events","html_url":"https://github.com/react-toolbox/react-toolbox/issues/652","id":166050686,"number":652,"title":"Discourage Binding Syntax for Input onChange Function Prop","user":{"login":"adrianmc","id":943555,"avatar_url":"https://avatars.githubusercontent.com/u/943555?v=3","gravatar_id":"","url":"https://api.github.com/users/adrianmc","html_url":"https://github.com/adrianmc","followers_url":"https://api.github.com/users/adrianmc/followers","following_url":"https://api.github.com/users/adrianmc/following{/other_user}","gists_url":"https://api.github.com/users/adrianmc/gists{/gist_id}","starred_url":"https://api.github.com/users/adrianmc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/adrianmc/subscriptions","organizations_url":"https://api.github.com/users/adrianmc/orgs","repos_url":"https://api.github.com/users/adrianmc/repos","events_url":"https://api.github.com/users/adrianmc/events{/privacy}","received_events_url":"https://api.github.com/users/adrianmc/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/react-toolbox/react-toolbox/labels/enhancement","name":"enhancement","color":"03A9F4"},{"url":"https://api.github.com/repos/react-toolbox/react-toolbox/labels/feedback%20wanted","name":"feedback wanted","color":"5319e7"},{"url":"https://api.github.com/repos/react-toolbox/react-toolbox/labels/help%20wanted","name":"help wanted","color":"43A047"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2016-07-18T09:08:59Z","updated_at":"2016-08-07T20:13:14Z","closed_at":null,"body":"I've been looking at the docs for the Input element and I think the suggested way of data binding is a little bit unwieldy. More specifically, I'm talking about how to handle the data you get from the Input components. I propose that we change the docs to reflect a better approach.\r\n\r\nFor context, I was inspired by [this gist](https://gist.github.com/ffxsam/21c1886e0e174e762d73aeb3541e7e26) which I got from [this Reddit post](https://www.reddit.com/r/webdev/comments/4tbha0/data_binding_code_in_9_javascript_frameworks/d5gas4o).\r\n\r\n# What we have now\r\n\r\nCurrently, the suggested way of using the Input component is to bind the data name with the function call:\r\n\r\n```jsx\r\nonChange={this.handleChange.bind(this, 'email')}\r\n```\r\n\r\nWe already know that the AirBnB style guide discourages using `.bind()` inside the render function ([source](https://github.com/airbnb/javascript/tree/master/react)). As explained [here](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md):\r\n\r\n> A bind call or arrow function in a JSX prop will create a brand new function on every single render. This is bad for performance, as it will result in the garbage collector being invoked way more than is necessary.\r\n\r\nIt also looks really bad in my opinion. This is what we have with the current approach:\r\n\r\n```jsx\r\nclass Component extends React.Component {\r\n  constructor(props) {\r\n    super(props);\r\n    this.state = {\r\n      email: '',\r\n      password: '',\r\n    };\r\n  }\r\n\r\n  handleChange(name, value) {\r\n    this.setState({\r\n      [name]: value,\r\n    });\r\n  }\r\n\r\n  render() {\r\n    return (\r\n      <div>\r\n        <Input\r\n          type=\"email\"\r\n          label=\"Email Address\"\r\n          value={this.state.email}\r\n          onChange={this.handleChange.bind(this, 'email')}\r\n          required\r\n        />\r\n        <Input\r\n          type=\"password\"\r\n          label=\"Password\"\r\n          value={this.state.password}\r\n          onChange={this.handleChange.bind(this, 'password')}\r\n          required\r\n        />\r\n      </div>\r\n    );\r\n  }\r\n}\r\n```\r\n\r\n# What I propose\r\n\r\nInstead, what we can do is add an `id` attribute to each input and bind `this` in the constructor. This allows us to simply write:\r\n\r\n```\r\nonChange={this.handleChange}\r\n```\r\n\r\n... which I think is a lot better and more closely resembles how functions are usually passed in as props. Revising the above example with this way of data binding:\r\n\r\n```jsx\r\nclass Component extends React.Component {\r\n  constructor(props) {\r\n    super(props);\r\n    this.handleChange = this.handleChange.bind(this);\r\n    this.state = {\r\n      email: '',\r\n      password: '',\r\n    };\r\n  }\r\n\r\n  handleChange(value, event) {\r\n    this.setState({\r\n      [event.target.id]: value,\r\n    });\r\n  }\r\n\r\n  render() {\r\n    return (\r\n      <div>\r\n        <Input\r\n          id=\"email\"\r\n          type=\"email\"\r\n          label=\"Email Address\"\r\n          value={this.state.email}\r\n          onChange={this.handleChange}\r\n          required\r\n        />\r\n        <Input\r\n          id=\"password\"\r\n          type=\"password\"\r\n          label=\"Password\"\r\n          value={this.state.password}\r\n          onChange={this.handleChange}\r\n          required\r\n        />\r\n      </div>\r\n    );\r\n  }\r\n}\r\n```\r\n\r\n# Ideally\r\n\r\nIn the [example gist](https://gist.github.com/ffxsam/21c1886e0e174e762d73aeb3541e7e26) from Reddit, the `handleChange()` function is more straight-forward than in my example.\r\n\r\nMy way:\r\n\r\n```jsx\r\nhandleChange(value, event) {\r\n  this.setState({\r\n    [event.target.id]: value,\r\n  });\r\n}\r\n```\r\n\r\nHis way:\r\n\r\n```jsx\r\nhandleChange({ target }) {\r\n  this.setState({\r\n    [target.id]: target.value,\r\n  });\r\n}\r\n```\r\n\r\nThis is because `onChange` for a traditional `<input>` element is usually called with just the event. In contrast, the Input component from the current implementation in React-Toolbox calls it with the value first and then the event:\r\n\r\n```jsx\r\nif (this.props.onChange) this.props.onChange(event.target.value, event);\r\n```\r\n\r\nSource: [Line 77 of Input.js](https://github.com/react-toolbox/react-toolbox/blob/dev/components/input/Input.js#L77)\r\n\r\nThis is why the example I gave is different from the one in the Gist. In the future, we may want to change this to better reflect the natural behaviour of an input element:\r\n\r\n```jsx\r\nif (this.props.onChange) this.props.onChange(event);\r\n```\r\n\r\nBut this will be a breaking change since I already have written my code to suit this approach. It's okay to keep going with what we have, but it should probably be better documented as well."},"comment":{"url":"https://api.github.com/repos/react-toolbox/react-toolbox/issues/comments/238105477","html_url":"https://github.com/react-toolbox/react-toolbox/issues/652#issuecomment-238105477","issue_url":"https://api.github.com/repos/react-toolbox/react-toolbox/issues/652","id":238105477,"user":{"login":"adrianmc","id":943555,"avatar_url":"https://avatars.githubusercontent.com/u/943555?v=3","gravatar_id":"","url":"https://api.github.com/users/adrianmc","html_url":"https://github.com/adrianmc","followers_url":"https://api.github.com/users/adrianmc/followers","following_url":"https://api.github.com/users/adrianmc/following{/other_user}","gists_url":"https://api.github.com/users/adrianmc/gists{/gist_id}","starred_url":"https://api.github.com/users/adrianmc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/adrianmc/subscriptions","organizations_url":"https://api.github.com/users/adrianmc/orgs","repos_url":"https://api.github.com/users/adrianmc/repos","events_url":"https://api.github.com/users/adrianmc/events{/privacy}","received_events_url":"https://api.github.com/users/adrianmc/received_events","type":"User","site_admin":false},"created_at":"2016-08-07T20:13:14Z","updated_at":"2016-08-07T20:13:14Z","body":"I did a bit of searching, and some of the suggestions here make a lot of sense:\r\n\r\nhttps://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md\r\n\r\nAccording to the ProTips section, we could pull in the Chip into its own ChipItem component and apply a function there instead.\r\n\r\nThis is probably a way better solution than what I was proposing above. I'm a little sleep-deprived."}}
{"actor":{"display_login":"adrianmc"}}