CODEWITHSUNDEEP
javascript
Tae
Tae

Reputation: 1695

Store the value, not the expression in JavaScript

First of all, I'm not so sure about how to ask this question. I have an array of elements:

var buttons = publishedWork.getElementsByTagName('button');

and what I what is that each button changes its content from ▶ to ◼ and viceversa by clicking. The thing is that I don't know how many buttons will be in total, I intend to do it with a for:

var currentButton;
for (var i = buttons.length; i;) {
    buttons[--i].onclick = function() {
        if (currentButton === buttons[i]) {
            currentButton.textContent = '▶';
            currentButton = null;
        } else {
            currentButton = buttons[i];
            currentButton.textContent = '◼';
        }
    }
}

But what this code does is that, no matter what button I click, it always changes the content of the first button, from which I get that is the expression buttons[i] the one that is stored in currentButton, and not the reference to the button itself.

So my question is:

is this a case to resolve by closures (a topic that I'm just beginning to grasp) or is there another solution?

Upvotes: 0

Views: 77

Answers (2)

Diode
Diode

Reputation: 25165

Use this or event.currentTarget inside click handler

....
buttons[--i].onclick = function(event) {


      // both `this` and `event.currentTarget` gives the clicked button here


}
....

.

var currentButton;
for (var i = buttons.length; i;) {
    buttons[--i].onclick = function() {
        if (currentButton === this) {
            ...
            currentButton = null;
        } else {
            currentButton = this;
            ...
        }
    }
}

and in your else condition you have to reset the current button first.

var currentButton;
for (var i = buttons.length; i;) {
    buttons[--i].onclick = function(event) {
        if (currentButton === this) {
            currentButton.textContent = '▶';
            currentButton = null;
        } else {
            if(currentButton){
                currentButton.textContent = '▶';
            }
            currentButton = this;
            currentButton.textContent = '◼';
        }
    }
}

Upvotes: 2

Flambino
Flambino

Reputation: 18773

Unless I'm mistaken, it looks like the common "defining an index-dependent function in a loop" issue. When the onclick function is invoked, it accesses the i variable. But when that happens, i has been all the way through the loop, and is 0. So all the click-handlers just see i == 0

To solve it, you can create a function that, in turn, creates the click-handler:

var currentButton;

function createClickHandler(index) {
    var button = buttons[index];
    return function() {
        if (currentButton === button) {
            currentButton.textContent = '▶';
            currentButton = null;
        } else {
            currentButton = button;
            currentButton.textContent = '◼';
        }
    };
}

for (var i = buttons.length; i;) {
    buttons[--i].onclick = createClickHandler(i);
}

Edit: Or use Diode's suggestion :)
I was focused on the "index in a closure" problem, but Diode's answer is much cleaner, and the better way to handle it

Upvotes: 3

Related Questions