0

Backstory:

• Varying dynamic items (buttons) will be generated and displayed in a single div.

• Each button is created from a unique object with a unique ID value.

Problem:

How do I get each generated and displayed button to retain, and then pass along when clicked, its unique "id"?

All of my efforts so far have gotten me results of "undefined" or displaying only the last generated id value, regardless of what button is clicked. Also things that target DOM elements don't seem to help as each of my unique items will not be inside it's own element. Rather just listed out in a single element.

As far as ideas/answers I am after straightforward/readability vs. speed/efficiency. I am also trying to keep as much of my functionality on the javascript side and rely on HTML for as little as possible beyond "displaying" things.

The following code is working as expected for me sans my question:

var allItems = [
 {id:1, name:"Space Gem", power:100},
 {id:14, name:"Time Gem", power:200},
 {id:22, name:"Reality Gem", power:300}
];

var map = {
 tile: [
  {id:22},
  {id:1}
 ]
}

onTile();

function onTile() {

    for ( var i = 0; i < map.tile.length; i++ ) {
        var itemId = map.tile[i].id;    

        for (var j = 0; j < allItems.length; j++) {
            if (itemId === allItems[j].id) {
                var itemName = allItems[j].name;

          var button = document.createElement("button");
                button.innerHTML = itemId + " " + itemName;
                document.getElementById("tile_display").appendChild(button);
                button.addEventListener ("click", get, false);
            }
        }   
    }

}

function get(itemId) {
    alert ("You clicked button with ID: " + itemId);
}

2 Answers2

1

The only problem I see is that you are passing the same event listener to each newly-created button. And what is more, you are passing the get function but not specifying an argument - which means that itemId will always be undefined when the function runs in response to a click. (I realise now this isn't true - itemId instead will refer to the Event object corresponding to the click event that's just happened - but this is no use to you in this case.)

So all you need to do, I think, is change:

button.addEventListener ("click", get, false);

to:

button.addEventListener ("click", function() {get(itemId);}, false);

EDIT: so this solves the "undefined" problem. But as you noticed, you are getting "id: 1" for both buttons. This is due to the fact that the event listener is a "closure" over its enclosing scope, which here is the onTile function. This means that, when you click the button and the event listener runs, it looks up the value of itemId, which it still has access to even though that scope would otherwise have been destroyed. But there is only one itemId in that scope, and it has whichever value it had when the function finished executing (here 1) - the same value for each event listener.

The simplest fix by far, assuming you are running in ES6-supporting browsers (which these days is all of them, although it always amazes me how many are still using IE which doesn't support it), is simply to change var ItemId = ... to let ItemId = .... Doing this gives ItemId a new scope, that of the loop itself - so you get a different value "captured" each time through the loop - exactly as you want.

In case you do need to support pre-ES6 browsers, you can perform the same "trick" without let, by enclosing the whole body of the outer for loop in a function (this creates a new scope each time), and then immediately invoking it, like this:

function onTile() {

    for ( var i = 0; i < map.tile.length; i++ ) {
        (function() {
            var itemId = map.tile[i].id;    

            for (var j = 0; j < allItems.length; j++) {
                if (itemId === allItems[j].id) {
                    var itemName = allItems[j].name;

                    var button = document.createElement("button");
                    button.innerHTML = itemId + " " + itemName;

                document.getElementById("tile_display").appendChild(button);
                    button.addEventListener ("click", function() 
                        {get(itemId);}, 
                    false);
                }
            }
        })();  
    }
}

function get(itemId) {
    alert ("You clicked button with ID: " + itemId);
}

Javascript closures, and in particular how they interact with loops like this, are a tricky topic which has caught many out - so there are loads of SO posts about it. JavaScript closure inside loops – simple practical example is an example, with the answer by woojoo66 being a particularly good explanation.

Robin Zigmond
  • 17,805
  • 2
  • 23
  • 34
  • No go. Sadly this is the approach that results in my aforementioned "displaying only the last generated id value, regardless of what button is clicked." – Curly Brackets Sep 20 '18 at 22:01
  • @CurlyBrackets - sorry, I hadn't tested it, but I see the problem now. Give me a few minutes to think of the best way around it, and I'll edit the answer. – Robin Zigmond Sep 20 '18 at 22:05
  • @CurlyBrackets - I've done a rather mammoth edit trying to explain what was happening and why, but the TLDR, if you just want to fix the code, is to replace your `var` by `let` when declaring `itemId` – Robin Zigmond Sep 20 '18 at 22:24
  • Are you serious right now? Changing var to let allows the variable to take that all important 'snapshot' of the value at that moment in time that I simply could not figure out how to get? Burned so many experimenting/researching hours on this. Unbelievable. Big green check mark for you sir. (Also did you mean "by enclosing the whole **inner** for loop in a function" by any chance. Your answer is so detailed I want to make sure it's perfect for future generations). – Curly Brackets Sep 21 '18 at 00:40
  • Don't worry, JS - and coding in general - can be tricky at times, and if you've not come across this issue before it's not at all obvious it's to do with scoping and closures. I did mean the outer loop but will edit to be more accurate and refer specifically to the *body* of the loop. The key thing here is that the `var itemId` declaration is inside the function, as well as the `addEventListener` where you use it. – Robin Zigmond Sep 21 '18 at 06:28
0

All that ever needs to happen here is to use the onClick = function() {} property for the newly created button and directly specify the itemId there like so:

button.onclick = function() {
  get(itemId);
}

You can easily implement this in a little function like make_button(itemId) { } (see below)

make_button(1);
make_button(2);
make_button(3);
make_button(4);
make_button(5);

function make_button(itemId) {

  var button = document.createElement("button");

  button.onclick = function() {
    get(itemId);
  }

  button.innerHTML = "button " + itemId;
  document.getElementById("mydiv").appendChild(button);

}



function get(_itemId) {
  alert("You picked button " + _itemId);
}
<div id="mydiv">
</div>

A much easier way to do this would be to do something like this:

var allItems = [{
    id: 1,
    name: "Space Gem",
    power: 100
  },
  {
    id: 14,
    name: "Time Gem",
    power: 200
  },
  {
    id: 22,
    name: "Reality Gem",
    power: 300
  }
];

var map = {
  tile: [{
      id: 22
    },
    {
      id: 1
    }
  ]
}

onTile();

function onTile() {

  for (var i = 0; i < map.tile.length; i++) {
    var itemId = map.tile[i].id;

    /* filter out only items in allItems[] that have id === itemId */
    var items = allItems.filter(item => item.id === itemId);

    /* loop through those few items and run function make_button */
    items.forEach(function(item) {
      make_button(item.id, item.name);

    });
  }
}
  /* avoid the use of function names such as 'get'  'set' and other commonly  used names as they may conflict with other scripts or native javascript */  

function make_button(itemId, itemName) {
  var button = document.createElement("button");
  button.innerHTML = itemId + " " + itemName;

  button.onclick = function() {
    get_clicked_tile(itemId); // changed name from 'get'
  };

  document.getElementById("tile_display").appendChild(button);

}

function get_clicked_tile(itemId) {
  alert("You clicked button with ID: " + itemId);
}
<div id="tile_display"></div>
mike510a
  • 2,102
  • 1
  • 11
  • 27
  • Going to be honest, looks like you swapped my inner loop with a filter function, which is cool, but far less readable to me/my current JS level. Just not familiar yet with the arrow functions and everything I read keeps saying these approaches cost speed, but I dunno. Secondly the OnClick approach vs the addEventListener seems identical to me but ironically perhaps a bit longer to pull off. Good call on the "get" and surprised you didn't call me out on using "map" as an array name. I had changed both in my code shortly after my OP. Interest piqued on "filter", it's def on my long list. – Curly Brackets Sep 23 '18 at 07:12
  • Forgot to add I am also new to forEach but it is my understanding it is basically shorthand for another for loop (part of the job my inner loop was performing). Will look into none-the-less. (Thank you for the //comments in your code) – Curly Brackets Sep 23 '18 at 07:16
  • The only reason I changed it was so that there wasnt two nested for loops, which can make things confusing to those trying to read the code... to me it seemed easier to read than having all of the nested functions.. sorry about that.. – mike510a Sep 23 '18 at 21:52
  • Dude you are 100% all good, I just wanted to (a) confirm that technically a forEach command is a 'for loop' and possibly taking up as much (more?) processing power... and (b) be transparent that it's not as clear to me and my level yet to really "know" what's going on under the hood if i don't see the loop. Regardless I am already peeking into your shortcuts/approach here and there, trust me. – Curly Brackets Sep 23 '18 at 23:51
  • Credit where credit is due, though a departure from the code I supplied, a specific you shared, `button.onclick = function() {getskibob(itemId);}` is for sure cleaner and clearer than my `button.addEventListener ("click", function() {getskibob(itemId);}, false);`. Stealing henceforward. – Curly Brackets Sep 24 '18 at 06:39