0

I'm interested in cycling through some div's I have on my page that have numbers in the ID.

This is what I have so far...

    var count = 1;

    while(count != 12) {
        $('#logo-'.concat(count).concat(' > img')).click(function() {
            if($('#info-'.concat(count)).hasClass('dropdown-active')) {
                $('#info-'.concat(count)).slideUp();
                $('#info-'.concat(count)).removeClass('dropdown-active');
            } else {
                $('#info-'.concat(count)).slideDown();
                $('#info-'.concat(count)).addClass('dropdown-active');
            }
            return false;
        });

        count++;
    }

The count seems to stop working when it reaches the if statement.

So the ID's on the page are logo-1, info-1, logo-2, info-2, etc...

James George Dunn
  • 523
  • 1
  • 6
  • 16
  • I would just like to add that the code works if I manually type in the numbers. I'm just looking for a more efficient way of doing it. – James George Dunn Jan 22 '15 at 16:10
  • You need to use a closure. See the linked duplicate. – Scimonster Jan 22 '15 at 16:11
  • 2
    Sidenote: `'#logo-'.concat(count).concat(' > img')` is same as `'#logo-' + count + ' > img')`, and the latter is arguably more readable. And [faster](http://jsperf.com/concat-vs-plus-vs-join). – kamituel Jan 22 '15 at 16:13

2 Answers2

6

You can do this more cleanly as follows:

while(count != 12) {
    $('#logo-' + count + ' > img').click(function() {
        var origin = $(this).parent(),
            targetId = '#info-' + origin[0].id.substring(5),
            target = $(targetId);
        if(target.hasClass('dropdown-active')) {
            target.slideUp();
            target.removeClass('dropdown-active');
        } else {
            target.slideDown();
            target.addClass('dropdown-active');
        }
        return false;
    });

    count++;
}

But it would be preferable to give all your logos the same class (say, "logo"), and then you can ditch the while loop:

$('.logo > img').click(function() {
     var origin = $(this).parent(),
         targetId = '#info-' + origin[0].id.substring(5),
         target = $(targetId);
     if(target.hasClass('dropdown-active')) {
         target.slideUp();
         target.removeClass('dropdown-active');
     } else {
         target.slideDown();
         target.addClass('dropdown-active');
     }
});

Edit: As Karl-André Gagnon points out in the comments, you could also use $('[id^="logo-"]') as an alternative to giving them a class, and still use the no-while-loop approach.

One alternative to parsing the numbers out of the IDs would be to store the number in a data-num attribute:

<div class='logo' data-num='1'>...</div>

And then instead of that var origin... stuff with the substring method, you would have:

var num = $(this).parent().data('num'),
    target = $('#info-' + num);
JLRishe
  • 99,490
  • 19
  • 131
  • 169
1

While JLRishe's answer is preferable, here's a largely academic demonstration of doing it with closures:

var count = 1;

while(count != 12) {
    (function(id){
        $('#logo-'.concat(id).concat(' > img')).click(function() {
            if($('#info-'.concat(id)).hasClass('dropdown-active')) {
                $('#info-'.concat(id)).slideUp();
                $('#info-'.concat(id)).removeClass('dropdown-active');
            } else {
                $('#info-'.concat(id)).slideDown();
                $('#info-'.concat(id)).addClass('dropdown-active');
            }
            return false;
        });
    }(count));

    count++;
}

By calling a function and passing the value as a parameter, you create a new scope and thus id will maintain it's value in the click function. I wouldn't necessarily recommend this approach (there are many inefficiencies in your code besides), but hopefully it's an interesting demonstration of closures and scope.

JDB
  • 25,172
  • 5
  • 72
  • 123