Dynamically adding div's and setting onclick value in JavaScript for loop

Question

I'm using vanilla JavaScript to create a photo carousel; reading in values from an external JSON file, then looping through the JSON data to dynamically add thumbnails, a title, and an onclick to the carousel div. All the images and titles work fine, but the onclick passes only the last value used regardless of which thumbnail is clicked.

Here's the JSON file

var Scenes = {
    "Title":["St. Mary Magdalene","Black Mountains","Pen Twyn Glas","Carreg Yr Ogof Cave","Beinn Narnain"],
    "Thumbnail":["TNStMary.jpg","TNMountains.jpg","TNPenTwynGlas.jpg","TNCarregYrOgof.jpg","TNBeinnNarnain.jpg"],
    "PanoSet":["001x","145+","183+","400x","500x"]
};

Here's the code that is dynamically adding the thumbnails, titles, and onclick.

var Scenes = {
    "Title":["St. Mary Magdalene","Black Mountains","Pen Twyn Glas","Carreg Yr Ogof Cave","Beinn Narnain"],
    "Thumbnail":["TNStMary.jpg","TNMountains.jpg","TNPenTwynGlas.jpg","TNCarregYrOgof.jpg","TNBeinnNarnain.jpg"],
    "PanoSet":["001x","145+","183+","400x","500x"]
};

var Repeat = (Scenes.Title.length);

for (i = 0; i < Repeat; i++) {
  NewDiv = document.createElement('div');
  NewDiv.id = 'Scene' + i;
  document.getElementById('Carousel').appendChild(NewDiv);
  document.getElementById('Scene' + i).className = "Scene";
  document.getElementById('Scene' + i).style.position = "absolute";
  document.getElementById('Scene' + i).style.top = "5px";
  document.getElementById('Scene' + i).style.left = 5 + (170 * i) + "px";
  document.getElementById('Scene' + i).onclick = function() {
    addScene(Scenes.PanoSet[i]);
  };

  NewDiv = document.createElement('div');
  NewDiv.id = 'SceneThumbnail' + i;
  document.getElementById('Scene' + i).appendChild(NewDiv);
  document.getElementById("SceneThumbnail" + i).className = "SceneThumbnail";
  document.getElementById("SceneThumbnail" + i).style.backgroundImage = "url(" + Scenes.Thumbnail[i] + ")";

  NewDiv = document.createElement('div');
  NewDiv.id = 'SceneTitle' + i;
  document.getElementById('Scene' + i).appendChild(NewDiv);
  document.getElementById("SceneTitle" + i).className = "SceneTitle";
  document.getElementById('SceneTitle' + i).innerHTML = Scenes.Title[i];
}

<div id='Carousel'><div>

This is the line that appears to be the problem (I included all above so as to give context)

document.getElementById('Scene'+i).onclick = function() {addScene(Scenes.PanoSet[i]);};

Which ever of the newly created divs I click on, the code "500x" is passed to my addScene function.

I'm expecting that I've over looked something simple. Can someone offer a solution?

You don't need to call `document.getElementById('Scene'+i)` over and over, you have the element at `NewDiv` just use that — Liam, Jul 24 '20 at 07:18
Does this answer your question? [JavaScript closure inside loops – simple practical example](https://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) — Billy Brown, Jul 24 '20 at 07:18
`i` is muted by you loop, so each click has the same reference to `i`. As above, this is a dupe — Liam, Jul 24 '20 at 07:19
Other than fixing the `i` you could also look into event delegation (onclick event listener on the parent element, get what you need from the event target). — pawel, Jul 24 '20 at 07:27

score 2 · Accepted Answer · answered Jul 24 '20 at 07:42

Classic JS gotcha. Do this:

document.getElementById('Scene'+i).onclick = ((id) => () =>addScene(Scenes.PanoSet[id]))(i);

You need to bind in the value of i, rather than a reference. The handler code doesn't get evaluated until it gets executed, so the value of i at that time will be the value it has when the loop has completed.

Doing it using an IIFE (immediately-invoked function expression) creates a context with the correct static value of i bound into the scope of the handler that it returns.

score 0 · Answer 2 · answered Jul 24 '20 at 07:37

You can find index like below. Inside onclick using this you will get clicked div. Then get id with this.getAttribute('id') and replace Scene with ''.

let x = this.getAttribute('id').replace('Scene', '');

Try it below.

let Scenes = {
  "Title":["St. Mary Magdalene","Black Mountains","Pen Twyn Glas","Carreg Yr Ogof Cave","Beinn Narnain"],
  "Thumbnail":["TNStMary.jpg","TNMountains.jpg","TNPenTwynGlas.jpg","TNCarregYrOgof.jpg","TNBeinnNarnain.jpg"],
  "PanoSet":["001x","145+","183+","400x","500x"]
};


var Repeat = (Scenes.Title.length);

for (i = 0; i < Repeat; i++) {
  NewDiv = document.createElement('div');
  NewDiv.id = 'Scene' + i;
  document.getElementById('Carousel').appendChild(NewDiv);
  document.getElementById('Scene' + i).className = "Scene";
  document.getElementById('Scene' + i).style.position = "absolute";
  document.getElementById('Scene' + i).style.top = "5px";
  document.getElementById('Scene' + i).style.left = 5 + (170 * i) + "px";
  document.getElementById('Scene' + i).onclick = function() {
    let x = this.getAttribute('id').replace('Scene', '');
    console.log(x);
    addScene(Scenes.PanoSet[x]);
  };

  NewDiv = document.createElement('div');
  NewDiv.id = 'SceneThumbnail' + i;
  document.getElementById('Scene' + i).appendChild(NewDiv);
  document.getElementById("SceneThumbnail" + i).className = "SceneThumbnail";
  document.getElementById("SceneThumbnail" + i).style.backgroundImage = "url(" + Scenes.Thumbnail[i] + ")";

  NewDiv = document.createElement('div');
  NewDiv.id = 'SceneTitle' + i;
  document.getElementById('Scene' + i).appendChild(NewDiv);
  document.getElementById("SceneTitle" + i).className = "SceneTitle";
  document.getElementById('SceneTitle' + i).innerHTML = Scenes.Title[i];


}

<div id='Carousel'></div>

Dynamically adding div's and setting onclick value in JavaScript for loop

2 Answers2