Unable to loop out event listeners for menu items

Question

I'm trying to assign some scrolling destinations for a menu for my one page application, the idea of this script is to prevent myself from repeating the same code over and over, but it's not quite working out the way I want it to.

Basically what happens is that destinations[i] becomes undefined and therefore I cannot access .dest of it. When I try just to console log destinations[i] I do get the desired result, but when I then go to click the buttons I can't scroll to anything since nothing is defined as the .offset value.

Why doesn't the value of destinations[i] remain after the loop and what can I do to make this work as intended?

var destinations = [
    {class: '.home', dest: $('html, body')},
    {class: '.lunch', dest: $('#menu-wrapper')},
    {class: '.catering', dest: $('#catering-wrapper')},
    {class: '.renting', dest: $('#renting-wrapper')},
    {class: '.karaoke', dest: $('#karaoke-wrapper')},
    {class: '.contact', dest: $('#site-footer')}
],
destLength = destinations.length,
i;

for (i = 0; i < destLength; i++) {
    $(destinations[i].class + ' a').on('click', function() {
    $('html, body').stop().animate({
        scrollTop: destinations[i].dest.offset().top
    }, 1000);
    });
}

dandavis · Accepted Answer · 2015-05-02T21:06:41.007

use functions, they have the scope you need:

var destinations = [
    {"class": '.home', dest: $('html, body')},
    {"class": '.lunch', dest: $('#menu-wrapper')},
    {"class": '.catering', dest: $('#catering-wrapper')},
    {"class": '.renting', dest: $('#renting-wrapper')},
    {"class": '.karaoke', dest: $('#karaoke-wrapper')},
    {"class": '.contact', dest: $('#site-footer')}
];

destinations.forEach(function(dest, i){ 
    $(dest['class'] + ' a').on('click', function() {
      $('html, body').stop().animate({
        scrollTop: dest.dest.offset().top
      }, 1000);
    });
});

forEach() also leaves no extra vars hanging around, gives you an index for var free, doesn't need a length, and looks a little cleaner to boot.

edit: added old-ie-safe prop access for "class", which is a reserved word in JS...

score 0 · Answer 2 · edited May 23 '17 at 12:14

0

I can't say for sure without seeing the surrounding code, but if you have the var in front of the declaration of destinations and it's inside of the loop, then destinations is deleted as soon as the loop is executed. See What is the purpose of the var keyword and when to use it (or omit it)?

edited May 23 '17 at 12:14

Community

1
1

answered May 02 '15 at 20:56

Mitchell Carroll

479
5
13

score 0 · Answer 3 · answered May 02 '15 at 21:05

As @dandavis suggested, foreach() seems more logical here, but for your code, when you loop until destLength it gives you the number of elements in destinations[], but elements numbering starts from 0, so you should use destLength-1

var destinations = [
    {class: '.home', dest: $('html, body')},
    {class: '.lunch', dest: $('#menu-wrapper')},
    {class: '.catering', dest: $('#catering-wrapper')},
    {class: '.renting', dest: $('#renting-wrapper')},
    {class: '.karaoke', dest: $('#karaoke-wrapper')},
    {class: '.contact', dest: $('#site-footer')}
],
destLength = destinations.length,
i;

for (i = 0; i < (destLength-1); i++) {
    $(destinations[i].class + ' a').on('click', function() {
    $('html, body').stop().animate({
        scrollTop: destinations[i].dest.offset().top
    }, 1000);
    });
}

Unable to loop out event listeners for menu items

3 Answers3