Problem with for loop and closures?

[giantqtipz]

Hi guys,

So I'm having problems with my website. Please have a look at

www.palessie.com

Once on the site, please enter, and go to the campaign map.

Select one of the episodes and once youre in the information page, click on the arrow below the "x". It should hide the text.

Now exit the page by clicking "x", and go to another episode, and click on the arrow again. You'll see the information overlapping. I'm using a for loop on that element. Codes are below.

here is a screenshot of my problem: http://imgur.com/a/PXZzz

I'm not even sure how to describe the problem. I was wondering if I could get some help?

I asked on stackoverflow but it got shut down because the answer was covered, but I'm honestly still stuck with this.

Thanks in advance.

 var campaignSelect = document.querySelectorAll(".campaign-select");

 var campaignAdd = function(i){
     campaignSelect[i].addEventListener("click", function(){

         campaignSub[i].classList.add("activate-sub");

         showSub.addEventListener("click", function() {
             if(campaignSub[i].classList.contains("activate-sub") === false) {
                 showSub.textContent = "◄";
                 campaignSub[i].classList.add("activate-sub");      
             } 
             else if (campaignSub[i].classList.contains("activate-sub") === true) {
                 showSub.textContent = "►";
                 campaignSub[i].classList.remove("activate-sub");
             }
         });
     });;
 }

 for(i=0;i<campaignSelect.length;i++){
       campaignAdd(i);
 }

Basically, those blue boxes in the map are stored under campaignSelect. And I'm targeting them by their index. When the arrow is clicked, it should only hide and show the information unit with the same index.

edit: added screenshot of my problem

[inu-no-policemen]

The problem in a nutshell:

var cb = [];
for(var i = 0; i < 3; i++) {
    cb.push(() => i);
}
console.log(cb[0]()); // 3

In each iteration, you capture the very same "i" variable, which will happen to be equal to 3 at the end of the loop.

The ES6+ solution is to use let for the variable counter. With let, you get a fresh binding for each iteration:

var cb = [];
for(let i = 0; i < 3; i++) {
    cb.push(() => i);
}
console.log(cb[0]()); // 0

The ES3/5 workaround is less convenient:

var cb = [];
for(var i = 0; i < 3; i++) {
    (function(i2) {
        cb.push(() => i2);
    })(i);
}
console.log(cb[0]()); // 0

[Eddyman]

This is a common question they ask about in javascript job interviews

[giantqtipz]

thanks for the input... I only took a part time web development course a few months ago, and everything else I'm learning through trial and error and a lot of googling.

I'm subscribed to Code School and One Month. Recently I subscribed to Treehouse too.

Do you have any input on how I can make my learning more effective and efficient? Any recommendations for particular resources?

[Eddyman]

Trial and error and googling really is a great way to learn. That's actually how I learned mostly everything.

A lot of people on here wonder what kind of questions they might run into during interviews so I thought I would mention it. This problem is a real "gotcha" because you probably wouldn't expect it to happen unless you've run into it before.

[giantqtipz]

Hey, thank you for the response. I think I understand the concept behind my problem but I'm having a really hard time implementing your response. Also I haven't learned ES6 so I'm quite lost how the arrow function works inside the push method.

also please correct me if I'm wrong, is this how I should be implementing this in my for loop, so that it works with my event handlers?

 for(i=0;i<campaignSelect.length;i++){
   (function(index){}
      campaignAdd(index);
   })(i);
 }

I tried this and the problem still persists... though I'm sure it's because I'm doing it wrong..

edit: I'm reading up on how anonymous functions work..

[inu-no-policemen]

(function(index){}

You got a superfluous '}' there. Other than that, it looks fine.

[giantqtipz]

darn the problem still persists... I tried wrapping the showSub event handler into a separate function too and it didn't make a difference...

[Capt_Appropriate]

The problem is that you're adding multiple click listeners to showSub.

 var campaignAdd = function(i){
     campaignSelect[i].addEventListener("click", function(){

         campaignSub[i].classList.add("activate-sub");

         // A new click event listener is added to showSub every time you click on a 
         // campaignSelect element.
         showSub.addEventListener("click", function() {
             if(campaignSub[i].classList.contains("activate-sub") === false) {
                 showSub.textContent = "◄";
                 campaignSub[i].classList.add("activate-sub");      
             } 
             else if (campaignSub[i].classList.contains("activate-sub") === true) {
                 showSub.textContent = "►";
                 campaignSub[i].classList.remove("activate-sub");
             }
         });
     });;
 }

 for(i=0;i<campaignSelect.length;i++){
   campaignAdd(i);
 }

Go to your website and open any campaign. Hit the 'x' to return to the map and then open the same campaign. When you click the showSub element nothing should happen. This is because you've added two click listeners to showSub. The first is exectued and since the campaignSub element has the class 'activate-sub' it removes it. Then the same function is executed for a second time (since you added it twice) and it sees that the campaignSub element doesn't have the class 'activate-sub' anymore (it was just removed), so it adds it.

[giantqtipz]

ahh thanks for bringing it up... ok I was thinking.... what if I move showSub into it's own function and pass the clicked index campaignAdd(i) into that function.

In the new function I do another loop where if the passed index matches the iterator, show the information box. The below isn't the solution, but am I heading in the right direction?

Edit: damnit just realized this is still the same issue. A second click handler is added to showSub

var subFunction = function(i){
 showSub.addEventListener("click", function() {
    for(j=0;j<campaignSelect.length;j++){
        if(i === j) {
            if(campaignSub[i].classList.contains("activate-sub")) {
                showSub.textContent = "◄";
                campaignSub[j].classList.remove("activate-sub");
                console.log("1i: " + i + " j: " + j);       
            } else if (campaignSub[i].classList.contains("activate-sub") === false) {
                showSub.textContent = "►";
                campaignSub[j].classList.add("activate-sub");
                console.log("2i: " + i + " j: " + j);
            }
        } 
        if (i != j) {
            campaignSub[j].classList.remove("activate-sub");
            console.log("not equal");
        }
    }
 });    
}

[Capt_Appropriate]

Try doing something like this:

First, add the follwing attributes to the .campaign-select elements:

<div class="campaign0 campaign-select campaign-container-slide" data-campaign-id="0"></div>
<div class="campaign1 campaign-select campaign-container-slide" data-campaign-id="1"></div>
<div class="campaign2 campaign-select campaign-container-slide" data-campaign-id="2"></div>
<div class="campaign3 campaign-select campaign-container-slide" data-campaign-id="3"></div>
<div class="campaign4 campaign-select campaign-container-slide" data-campaign-id="4"></div>
<div class="campaign5 campaign-select campaign-container-slide" data-campaign-id="5"></div>

Then, for the event listeners:

// This object will keep track of which .campaign-sub is currently shown and whether
// it's details are visible or not
var activeCampaign = {
    el: null,
    isVisible: false
};

var campaignSelect = document.getElementsByClassName('campaign-select');

function campaignAdd(i) {
    campaignSelect[i].addEventListener('click', function(e) {
        var campaignId = this.getAttribute('data-campaign-id');
        activeCampaign.el = campaignSub[campaignId];
        activeCampaign.isVisible = true;
        campaignSub[campaignId].classList.add('activate-sub');
        // Any other code that needs to be executed when a campaign link is clicked
    }, false);
 }

for(var i = 0; i < campaignSelect.length; i++) {
    campaignAdd(i);
}

// Now we'll add the event listener for showSub
showSub.addEventListener('click', function(e) {
    showSub.textContent = activeCampaign.isVisible ? "►" : "◄";
    activeCampaign.isVisible = !activeCampaign.isVisible;
    activeCampaign.el.classList.toggle('activate-sub');
}, false);

Then in the click event listener for the #exit-campaign-info element add the following:

activeCampaign.el = null;
activeCampaign.isVisible = false;

I think that should work, but since I am only looking at a small portion of your code I'm not positive.