11

I've this layout that was created dynamically:

for (let i = 1; i < 10; i++) {
  document.querySelector('.card-body').innerHTML += `<div class="row" id="img_div">
        <div class="col-12 col-sm-12 col-md-2 text-center">
          <img src="http://placehold.it/120x80" alt="prewiew" width="120" height="80">
        </div>
        <div id="text_div" class="col-12 text-sm-center col-sm-12 text-md-left col-md-6">
        <h4 class="name"><a href="#" id="title` + i + `">Name</a></h4>
          <h4>
            <small>state</small>
          </h4>
          <h4>
            <small>city</small>
          </h4>
          <h4>
            <small>zip</small>
          </h4>
        </div>
        <div class="col-12 col-sm-12 text-sm-center col-md-4 text-md-right row">
        </div>
      </div>
     `
  document.getElementById("title" + i).addEventListener('click', function() {
    console.log(i)
  });
}
<link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css" integrity="sha384-ggOyR0iXCbMQv3Xipma34MD+dH/1fQ784/j6cY/iJTQUOhcWr7x9JvoRxT2MZw1T" crossorigin="anonymous">
<div class="card-body">
  <!-- person -->
</div>

And I want to get the event click on each h4 class="name" and show a log with the number i related.

However, console.log shows only the last i related (i=9 in this case), and doesn't work with the other i numbers. Why does this happen? What do I have to do?

double-beep
  • 4,567
  • 13
  • 30
  • 40
Luiz Adorno
  • 117
  • 8
  • 3
    Maybe use [Document.createElement()](https://developer.mozilla.org/en-US/docs/Web/API/Document/createElement) instead of passing html-string & you'll have a much easier time & i think you'll end up with better code in the end. – admcfajn Feb 14 '20 at 22:58
  • 1
    you''ll have to loop through your nodes again and attach an event you cannot attach into a string – Eugen Sunic Feb 14 '20 at 22:59
  • Nice question, it's kinda tricky for sure, I think it might be possible to attatch an event post rendering.. For example after your `for` loop calling a method to apply events to those elements, but I don't know. will jsFiddle to test. – Pogrindis Feb 14 '20 at 23:02
  • Created a delegated event handler for `.card-body` and check if the element (`target`) is an anchor with and ID that starts with `title`. – hungerstar Feb 14 '20 at 23:03
  • 1
    Ah, just about to post an answer. Voted to reopen. Closures aren't necessary, the problem is `innerHTML += ...` killing the previously set event listeners, not the closure. Use `document.createElement` instead. See [this thread](https://stackoverflow.com/questions/38361875/element-innerhtml-getting-rid-of-event-listeners) – ggorlen Feb 14 '20 at 23:05
  • You are attaching the event listener inside the loop. You loose the event listener when you recreate the elements. 2 solutions: move the creation of the event listeners outside the first loop, or instead of building the entire innerhtml, build each div and append it to the dom. – Diogo Gomes Feb 14 '20 at 23:12

2 Answers2

8

Using += on innerHTML destroys event listeners on the elements inside the HTML. The correct way to do this is to use document.createElement. Here's a minimal, complete example:

for (let i = 1; i < 10; i++) {
  const anchor = document.createElement("a");
  document.body.appendChild(anchor);
  anchor.id = "title" + i;
  anchor.href= "#";
  anchor.textContent = "link " + i;
  document.getElementById("title" + i)
    .addEventListener("click", e => console.log(i));
}
a {
  margin-left: 0.3em;
}

Of course, the document.getElementById isn't necessary here since we just created the object in the loop block. This saves potentially expensive tree walks.

for (let i = 1; i < 10; i++) {
  const anchor = document.createElement("a");
  document.body.appendChild(anchor);    
  anchor.href= "#";
  anchor.textContent = "link " + i;
  anchor.addEventListener("click", e => console.log(i));
}
a {
  margin-left: 0.3em;
}

If you have arbitrary chunks of stringified HTML as in your example, you can use createElement("div"), set its innerHTML once to the chunk and add listeners as needed. Then append the divs as children of your container element.

for (let i = 1; i < 10; i++) {
  const rawHTML = `<div><a href="#" id="link-${i}">link ${i}</a></div>`;
  const div = document.createElement("div");
  document.body.appendChild(div);
  div.href= "#";
  div.innerHTML = rawHTML;
  div.querySelector(`#link-${i}`)
    .addEventListener("click", e => console.log(i));
}
ggorlen
  • 33,459
  • 6
  • 59
  • 67
  • 1
    This is the approach I was considering, and I would hands down give this the solution, but the other answer is great too. – Pogrindis Feb 14 '20 at 23:09
  • Thanks for de answer, but i tried to build the layout with createElement and i was unsuccessful – Luiz Adorno Feb 14 '20 at 23:15
  • 1
    No problem. Hopefully you realize if you have a big chunk of HTML, you can create a root element container like a `
    `, then `div.innerHTML += yourHugeChunkOfHTML`. So there's no reason this shouldn't work for any use case.
    – ggorlen Feb 14 '20 at 23:16
  • 1
    I will try to build this layout with your recommendation, I am afraid to use "document.getElementById" several times and slow the performance – Luiz Adorno Feb 14 '20 at 23:23
7

innerHTML redraws the full html, as a result all the previously attached events are lost. Use insertAdjacentHTML()

for(let i=1;i<10;i++){
    document.querySelector('.card-body').insertAdjacentHTML('beforeend',`<div class="row" id="img_div">
        <div class="col-12 col-sm-12 col-md-2 text-center">
          <img src="http://placehold.it/120x80" alt="prewiew" width="120" height="80">
        </div>
        <div id="text_div" class="col-12 text-sm-center col-sm-12 text-md-left col-md-6">
        <h4 class="name"><a href="#" id="title`+i+`">Name</a></h4>
          <h4>
            <small>state</small>
          </h4>
          <h4>
            <small>city</small>
          </h4>
          <h4>
            <small>zip</small>
          </h4>
        </div>
        <div class="col-12 col-sm-12 text-sm-center col-md-4 text-md-right row">
        </div>
      </div>
     `);
   document.getElementById("title"+i).addEventListener('click', function () {
    console.log(i)
  });
}
<link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css" integrity="sha384-ggOyR0iXCbMQv3Xipma34MD+dH/1fQ784/j6cY/iJTQUOhcWr7x9JvoRxT2MZw1T" crossorigin="anonymous">
<div class="card-body">
<!-- person -->
</div>
Mamun
  • 62,450
  • 9
  • 45
  • 52
  • 3
    Wow.. I've been messing around with simular things and never once came accross `insertAdjacentHTML` - Everydays a schoolday, really nice. – Pogrindis Feb 14 '20 at 23:08
  • 1
    Very cool and +1, but it still seems expensive and unnecessary to have to use `document.getElementById` immediately after adding the HTML. If you're adding hundreds of elements, this is a lot of traversing work. – ggorlen Feb 14 '20 at 23:15
  • @ggorlen I kinda agree on the perf area out of interest would using the class `name` and attatching the event to just that class while passing through the element (or maybe just reading from the event itself) be more efficient ? – Pogrindis Feb 14 '20 at 23:18
  • 1
    Not sure. Depends on the actual usage--my suggestion could be premature optimization. You'd have to profile/benchmark and see. But if `document.createElement` can be used, I think that's generally the best baseline approach. Another possible improvement to this post is to use two loops: one to build all the HTML, then another to grab all of the elements using a class in one `document.querySelectorAll` call and add the event listeners. – ggorlen Feb 14 '20 at 23:23
  • 1
    Definitely worth a benchmark, I can't think of anything better to do tonight! :) – Pogrindis Feb 14 '20 at 23:24