1

I have this code. If you click on the next button then click on the prev then the next, then the previous the index output gets mixed up.

Eg. When page loads, index is 0. then when I click next the index is 1. Then I click prev and it goes back to 0. So far so good, but when I click next again it goes to 2 and this messes eveything up.

Think it may be due to...

if (index < 0) {
        index = lengthMinusOne;
};

Been working on this for days. Please help. Very much appreciated.

//Image Gallery
var imgs = [
    ['images/test1.jpg', 'Test1', 'sdfsdfsdfsdf', 'light'],
    ['images/test2.jpg', 'Test2', 'sdfsdfsdf', 'light'],
    ['images/Test3.jpg', 'Test3', 'sdfsdfsdfsd.', 'dark']
];

var cnt = imgs.length;
var lengthMinusOne = cnt - 1,
    index = 0,
    fadeSpeed = 1000;

preload_image_object = new Image();
var i = 0;
for (i = 0; i <= cnt; i++)
preload_image_object.src = imgs[i];
$("#txt h1").text(imgs[0][1]);
$("#txt #desc p").text(imgs[0][2]);
var ld = imgs[0][3];
if (ld == "dark") {
    $("body").addClass("dark");
};
var firstImg = $('<img />');
$(firstImg).attr('src', imgs[0][0]);
$('#supersized').append(firstImg);
$(firstImg).hide().fadeIn(fadeSpeed);
$("#prev-photo").bind('click', prev);

function prev() {
    index--;
    $('#prev-photo,#next-photo').unbind();
    if (index < 0) {
        index = lengthMinusOne;
    };
    var ld = imgs[index][3];
    if (ld == "dark") {
        $("body").addClass("dark");
    } else {
        $("body").removeClass("dark");
    };
    oldImg = $('#supersized img').addClass('old');
    $("#txt h1").text(imgs[index][1]).hide().fadeIn();
    $("#txt #desc p").text(imgs[index][2]).hide().fadeIn();
    var img = new Image();
    $(img).load(function () {}).error(function () {}).attr('src', imgs[index][0]);
    $('#supersized').append(img);
    $('#supersized img').css('left', '0');
    $(img).hide().fadeIn(fadeSpeed, function () {
        oldImg.remove();
        $('#prev-photo').bind('click', prev);
        $('#next-photo').bind('click', prev);
    });
    console.log(index);
};
$("#next-photo").bind('click', next);

function next() {
    index++;
    $('#next-photo,#prev-photo').unbind();
    if (index > lengthMinusOne) {
        index = 0
    };
    var ld = imgs[index][3];
    if (ld == "dark") {
        $("body").addClass("dark");
    } else {
        $("body").removeClass("dark");
    };
    oldImg = $('#supersized img').addClass('old');
    $("#txt h1").text(imgs[index][1]).hide().fadeIn();
    $("#txt #desc p").text(imgs[index][2]).hide().fadeIn();
    var img = new Image();
    $(img).load(function () {}).error(function () {}).attr('src', imgs[index][0]);
    $('#supersized').append(img);
    $('#supersized img').css('left', '0');
    $(img).hide().fadeIn(1300, function () {
        oldImg.remove();
        $('#next-photo').bind('click', next);
        $('#prev-photo').bind('click', prev);
    });
    console.log(index);
};
Improve this question
2
  • Haven't looked too closely at the code, just read your description. Have you considered just pushing and popping your current images off a stack. This would eliminate having to keep track of indexes. Just a thought and good luck. Commented Mar 31, 2011 at 11:47
  • It's a circular showreel so I need to keep track of index so I can loop back to first/last image. Commented Mar 31, 2011 at 13:51

1 Answer 1

Reset to default
0

You are binding both handlers to the prev function when reapplying them in prev.

$(img).hide().fadeIn(fadeSpeed, function () {
    oldImg.remove();
    $('#prev-photo').bind('click', prev);
    $('#next-photo').bind('click', prev);
});

Should be:

$(img).hide().fadeIn(fadeSpeed, function () {
    oldImg.remove();
    $('#prev-photo').bind('click', prev);
    $('#next-photo').bind('click', next);
});

I would abstract this code (and any other duplicate code) into it's own function and call it from both prev and next.

Improve this answer
5
  • Perfect. So quick. Can't thank you enough. Could you please explain "abstract this code" and give me a starter on it? Commented Mar 31, 2011 at 11:48
  • @Model -- look at your functions. Much of them are the same except the values of the arguments to the functions you're calling. You could create a function containing the duplicate code using function parameters as replacements for the arguments that are different. Call this function from both prev and next with the appropriate actual values. One fundamental tenet of programming is "DRY - Don't Repeat Yourself" -- if you find yourself writing the same code, you should find a way to share what you've already written instead. Commented Mar 31, 2011 at 11:54
  • @Model - you might also consider just hiding/showing the prev/next buttons instead of unbinding/rebinding the handlers. If a button doesn't do anything, I'd prefer not to have it in the interface. If that doesn't work, then I'd apply some sort of CSS to indicate that it's "disabled". Commented Mar 31, 2011 at 11:58
  • I'm a bit of a novice. I assume you are talking about the prev() and next() functions. Would be great to see my code above optimised using the DRY method. It would be great if you could spend a sec or two to get me started. A big ask I know. Commented Mar 31, 2011 at 11:58
  • Thanks again. The binding is just to stop people clicking on the buttons while the image is loading in. Commented Mar 31, 2011 at 12:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.