I have a react ponent which renders a list item with individual OnClick.

In order to find out which item was clicked, the handler accepts a parameter. The handler does get invoked - but no matter which item is clicked - console always logs item3 (as if item3 is clicked). What am I doing wrong here?

class Item {
    constructor(props) {
        super(props);
        this.onItemClickHandler = this.onItemClickHandler.bind(this)
    }

    onItemClickHandler (itemName) {
        console.log("Clicked " + itemName)
    }

    render() {
        this.items = ["item1", "item2", "item3"]
        var lis = []
        for (var liName in this.items) {
            var liName2 = this.items[liName]
            console.log("Adding " + this.items[liName])
            lis.push(<li className="item-ListItem" key={this.items[liName]} onClick={() => this.onItemClickHandler(this.items[liName])}><span><a href="#">{this.items[liName]}</a></span></li>)
        }

        return (
          <div className="item">
            <label className="item-Header"><u>items</u></label>
            <ul className="item-List"> 
            {lis}
            </ul>

          </div>
        );
    }

This line:

onClick={() => this.onItemClickHandler(this.items[liName])}>

appears to be correct.

I have a react ponent which renders a list item with individual OnClick.

In order to find out which item was clicked, the handler accepts a parameter. The handler does get invoked - but no matter which item is clicked - console always logs item3 (as if item3 is clicked). What am I doing wrong here?

class Item {
    constructor(props) {
        super(props);
        this.onItemClickHandler = this.onItemClickHandler.bind(this)
    }

    onItemClickHandler (itemName) {
        console.log("Clicked " + itemName)
    }

    render() {
        this.items = ["item1", "item2", "item3"]
        var lis = []
        for (var liName in this.items) {
            var liName2 = this.items[liName]
            console.log("Adding " + this.items[liName])
            lis.push(<li className="item-ListItem" key={this.items[liName]} onClick={() => this.onItemClickHandler(this.items[liName])}><span><a href="#">{this.items[liName]}</a></span></li>)
        }

        return (
          <div className="item">
            <label className="item-Header"><u>items</u></label>
            <ul className="item-List"> 
            {lis}
            </ul>

          </div>
        );
    }

This line:

onClick={() => this.onItemClickHandler(this.items[liName])}>

appears to be correct.

• javascript
• reactjs

• Check this stackoverflow./questions/47395368/… – Shubham Khatri Commented Dec 2, 2017 at 7:55
• 1 You are using var in that for loop. Use let keyword instead. Scope issues with var. See : github./getify/You-Dont-Know-JS/blob/master/… – ArchNoob Commented Dec 2, 2017 at 8:15
• Using let solved the issue and was the most minimal change in my current code. Thanks! – justanothercoder Commented Dec 3, 2017 at 5:38

3 Answers 3

The issue is that you are not capturing the value of this.items[liName] correctly because by the time you reach the third item iteration the onClick handler will always have the value of this.items[liName] set to the third item.

The solution for that is using closure to capture the value correctly, i edited your code and created a fully working example in this link

https://codesandbox.io/s/3xrp6k9yvp

Also the example code is written below with the solution

class App extends Component {
  constructor(props) {
    super(props);
    this.onItemClickHandler = this.onItemClickHandler.bind(this);
  }

  onItemClickHandler(itemName) {
    console.log("Clicked " + itemName);
  }

  render() {
    this.items = ["item1", "item2", "item3"];
    var lis = [];
    for (var liName in this.items) {
      var liName2 = this.items[liName];
      console.log("Adding " + this.items[liName]);

      //the clickHandler function here is the solution we created a function that get executed immediately each iteration and return a new function that has the correct value of `this.items[liName]` saved
      var clickHandler = (item => {
        return event => {
          this.onItemClickHandler(item);
        };
      })(this.items[liName]);

      lis.push(
        <li
          className="item-ListItem"
          key={this.items[liName]}
          onClick={clickHandler} // here we use the clickHandler function directly
        >
          <span>
            <a href="#">{this.items[liName]}</a>
          </span>
        </li>
      );
    }

    return (
      <div className="item">
        <label className="item-Header">
          <u>items</u>
        </label>
        <ul className="item-List">{lis}</ul>
      </div>
    );
  }
}

For more info and examples about closures check this link

Edit We can use let in ES6 instead of var in our for loop as mentioned by @ArchNoob because using let will make the liName block scoped

Please take care of indentation while posting the code. Its very difficult to understand without that. You have to make use of closure. whenever the loop gets over liName variable gets set to last index as scope chain will keep the liName value to last one. The solution is not make a new scope between handler and click handler function where it is callled. Here is the solution:

class Test extends React.Component {
  constructor(props) {
    super(props)
    this.onItemClickHandler =             
    this.onItemClickHandler.bind(this)
  }

  onItemClickHandler(itemName) {
    debugger
   console.log("Clicked " + itemName)
  }
  
  render() {
    this.items = ["item1", "item2", "item3"]
    var lis = []
    for (var liName in this.items) {
      var liName2 = this.items[liName]
      console.log("Adding " + this.items[liName])
      debugger
      lis.push( <li className = "item-ListItem"
                  key = {
                    this.items[liName]
                  }
                  onClick = {
                    ((item) => {
                      return () => this.onItemClickHandler(item)
                    })(this.items[liName])
                  }
                 >
                  <span>
                    <a href = "#"> {this.items[liName]} </a>
                  </span> 
                </li>
      )
    }
    
    return (
      <div>
        <div className="item">
        <label className="item-Header">
         <u>items</u>
        </label>
        <ul className="item-List" >
          {lis}
        </ul>
      </div>
      </div>
    )
  }
}

ReactDOM.render(<Test />, document.getElementById("root"))

<script src="https://cdnjs.cloudflare./ajax/libs/react/15.1.0/react.min.js"></script>
<script src="https://cdnjs.cloudflare./ajax/libs/react/15.1.0/react-dom.min.js"></script>
<div id="root"></div>

I will do it the remended way. First make a separate ponent of list-item. To render list in react Js

//Handler
clickHandler=(id)=>{
    console.log(`Clicked on Item with Id ${id}`);
}

//render method
render(){
  let items = null;
  if(this.state.isAnyItem){
   <div className="items-list">
      {
        this.state.items.map((item)=>{
          <Item key={item.id} item={item} click={(item.id)=>this.clickHandler(item.id)}/>
         })
       }
   </div>
  }

return (
   <div>
     {/*Some JSX here*/}
        {items}
    </div>
  )
}

And Now the Item Component like

<div onClick={props.click}>
    {/*Some JSX Here*/}
    <h3>{item.name}</h3>
</div>