up vote 3 down vote favorite

Can someone please help to rewrite / tidy up this? 

// News Article Slideshow
var periodToChangeSlide = 5000;
var pp_slideshow = undefined;
var currentPage = 0;

$('#news-feature-img-wrap li').css('display', 'list-item').slice(1).css('display', 'none');
$('#news-items li:first').addClass('active');

$("#news-feature-wrap #news-items li").click(function () {
    $(this).parent().addClass('active');
    $(this).parent().siblings().removeClass('active');

    var index = $(this).parent().index();
    var toShow = $("#news-feature-wrap #news-feature-img-wrap li").eq(index);
    toShow.show();
    toShow.siblings().hide();
    currentPage = index;
    $.stopSlideshow();
});

$.startSlideshow = function () {
    if (typeof pp_slideshow == 'undefined') {
        pp_slideshow = setInterval($.startSlideshow, periodToChangeSlide);
    } else {
        $.changePage();
    }
}

$.stopSlideshow = function () {
    clearInterval(pp_slideshow);
    pp_slideshow = undefined;
}
$.changePage = function () {
    var numSlides = $('#news-feature-wrap #news-feature-img-wrap li').length;
    currentPage = (currentPage + 1) % numSlides;
    var menu = $('#news-feature-wrap #news-items li').eq(currentPage);
    menu.addClass('active');
    menu.siblings().removeClass('active');

    var toShow = $("#news-feature-wrap #news-feature-img-wrap li").eq(currentPage);
    toShow.show();
    toShow.siblings().hide();
}

$.startSlideshow();
share|improve this question
2  
@palacsint done! – Jordy Vialoux Jul 23 '12 at 2:13
It would be nice to have example html. – Inkbug Jul 23 '12 at 5:37
@JordyVialoux: Thanks :) – palacsint Jul 23 '12 at 8:26
@JordyVialoux You should use === by default, and not ==. – ANeves Nov 20 '12 at 17:09

1 Answer

up vote 0 down vote

Without HTML we could not do too much, but here is what I noticed right away.

I would definately use more of the chaining that is provided with jQuery.

$("#news-feature-wrap #news-items li").click(function () {
    $(this).parent().addClass('active').siblings().removeClass('active');

    var index = $(this).parent().index();
    $("#news-feature-wrap #news-feature-img-wrap li").eq(index).show().siblings().hide();
    currentPage = index;
    $.stopSlideshow();
});

This is more so taking advantage of the chaining in jQuery. It will not change performance and is strictly aesthetic.

share|improve this answer

Your Answer

 
or
required, but never shown
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.