CODEWITHSUNDEEP
javascriptjquery
Chrillewoodz
Chrillewoodz

Reputation: 28368

Unable to loop out event listeners for menu items

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);
    });
}

Upvotes: 0

Views: 35

Answers (3)

dandavis
dandavis

Reputation: 16726

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...

Upvotes: 2

Lupin
Lupin

Reputation: 1244

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);
    });
}

Upvotes: 0

Mitchell Carroll
Mitchell Carroll

Reputation: 489

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)?

Upvotes: 0

Related Questions