CODEWITHSUNDEEP
javascriptarraysif-statement
TTT
TTT

Reputation: 37

Array value as condition of if else- JS

I've been trying to understand why whenever value of the array I click, it always add the class "foo".

Example: I clicked on London (cities[1], right?) and it added the class foo. 

var cities = [
document.getElementById('Paris'),
document.getElementById('London'),
document.getElementById('Berlin')
];


for (var i = 0; i < cities.length; i++) {
  cities[i].onclick = test;

  function test(){
    if(cities[i] === cities[0]) {
      el.classList.add("foo");
    }
  }
}

Upvotes: 2

Views: 3100

Answers (4)

Moshe Bildner
Moshe Bildner

Reputation: 471

EDIT: my original answer was incorrect, this updated one is right. addEventListener returns nothing. Instead, you should use some kind of wrapper to add and remove your listeners, again so that you don't waste resources on listeners that you aren't using:

function on (element, eventName, callback) {
    element.addEventListener(eventName, callback);
    return function unregister () {
        element.removeEventListener(callback);
    }
}

function test (event) {
    if (event.currentTarget===cities[0]) {
        event.target.classList.add('foo');
    }
}

var listenerRemovers = cities.map(function (city) {
    return on(city, 'click', test);
});

Now you can remove any of these listeners by calling the corresponding function in your listenerRemovers array:

listenerRemovers.forEach(function (unRegisterFunc) { unRegisterFunc(); });

ORIGINAL WRONG ANSWER:

For what it's worth, you're probably better off using .map in a case like this, since best practice is to keep a reference to the event listeners so you can cancel them if needed.

function test (event) {
    if (event.currentTarget===cities[0]) {
        event.target.classList.add('foo');
    }
}

var listenerHandlers = cities.map(function (city) {
  return city.addEventListener('click', test);
});

Upvotes: 2

gen_Eric
gen_Eric

Reputation: 227240

This is happening because you are setting the event functions inside a loop. Each function is using the same value of i.

Try to use this instead of trying to cities[i] inside the function.

function test(){
    if(this === cities[0]) {
      el.classList.add("foo");
    }
}

Upvotes: 1

Downhillski
Downhillski

Reputation: 2655

The easiest approach to achieve this functionality is to use jQuery, here is the idea:

  1. In html tags, give those cities a common class, e.g. class="city"
  2. $('.city').click(function(){$('.city').addClass('foo')});

jQuery saves you more time and coding efforts.

Upvotes: 0

Joel Jeske
Joel Jeske

Reputation: 1647

The problem is you are trying to assign a function to a DOM attribute. You are not registering a listener but modifying the DOM. If you wish to do it this way, you must assign the onclick as cities[i].onclick = 'test()'

Also, you should move the function test outside of the for loop to look like the following. The problem is the function test is being declared many times, each with a different 'i' value. 

for (var i = 0; i < cities.length; i++) {
  cities[i].onclick = 'test(this)';
}

function test(el){
 if(cities[i] === cities[0]) {
    el.classList.add("foo");
 }
}

Upvotes: -1

Related Questions