Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#9685 Updated docs for event handling #9730

Closed
wants to merge 5 commits into from
Closed

#9685 Updated docs for event handling #9730

wants to merge 5 commits into from

Conversation

jordanpapaleo
Copy link

Fixes #9624

I added an explanation and an example of using .bind to the docs to pass arguments to event handlers.

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small comments, but otherwise great! Do you mind updating?


```js{2-5,14}
class List extends React.Component {
handleListClick (item, i, e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no space before the paren please (same for render below)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you also change "e" to "event" for clarity?

@@ -139,3 +139,29 @@ class LoggingButton extends React.Component {
```

The problem with this syntax is that a different callback is created each time the `LoggingButton` renders. In most cases, this is fine. However, if this callback is passed as a prop to lower components, those components might do an extra re-rendering. We generally recommend binding in the constructor or using the property initializer syntax, to avoid this sort of performance problem.

You can also use the `bind` method to pass arguments to your event handlers. Like you saw above, the first argument for the bind method will be the scope you want bound to your function. The last argument received by your event handler will always be the synthetic event. You have the ability to pass any number of arguments after `this` and before the event to your event handler:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do:

You can also use the `bind` method to pass arguments to your event handlers, which is particularly useful for knowing which item in a list was clicked. As shown above, the first argument

<ul>
{listItems.map((item, i) => {
return (
<li key={`item-${i}`}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be just key={i}.

@jordanpapaleo
Copy link
Author

Hi @sophiebits! All of your suggestions have been implemented.

I did one additional thing in the docs. i added an example of the nested function event handling. I did a survey a few months ago about event handling and discovered that this seems to be the preferred method by many people. It totally makes sense too and I have been doing this ever since.

Thank you for looking at my PR :)

}
```

There is one more way to do event binding that does not cause a `.bind()` to be called every on every render. You can set your event handler to a method that is an arrow function which returns nested arrow function. The ES6 syntax looks a little funny at first but it totally makes sense once you get what it is doing. The first arrow function is given the arguments you would like to access in your handler. It gets invoked immediately during the first render and returns the inner arrow function. The inner arrow function is what will be handling the click. The inner arrow function has access to the parameters passed to the outer arrow function. In JavaScript, nested functions have access to variables declared in their outer scope. It will only be called when the event occurs giving you a clean event handler:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading this over a few times, I think this is a little too complicated for this section. It's not a common pattern in my experience and also is something that someone familiar with JS functions should be able to figure out themselves if they want. Can we remove it?


```js{2-5,14}
class List extends React.Component {
handleListClick (item, i, event) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove the space before the paren here and in render () {

@jordanpapaleo
Copy link
Author

jordanpapaleo commented Sep 12, 2017 via email

@jordanpapaleo
Copy link
Author

@sophiebits Any feedback on my comment? Ill make the change as soon as I hear from you :)

@bvaughn
Copy link
Contributor

bvaughn commented Oct 8, 2017

Thank you for filing this PR!

I'm sorry to be the bearer of bad news, but the documentation and source code for reactjs.org now lives in a different repository: reactjs/reactjs.org. (For more info on why we made this move, see issue #11075.)

Would you be willing to re-open this PR on the new repo? I promise we'll review it quickly!

PS It may also help to link to this PR for the past discussion thread.

@bvaughn bvaughn closed this Oct 8, 2017
@jordanpapaleo
Copy link
Author

jordanpapaleo commented Oct 9, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants