Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm trying to come up with the most basic example of making a JQuery slide show where you click on the current image you're viewing and you cycle through a gallery of photos. I know its probably not the most basic example, because if I want to add a new image I have to code more JQuery. Is there a more abstract approach where I don't have to code JQuery in terms of div id's and let classes take care of the work? Here is my JQuery

$(document).ready(function() {
$("#pic1").click(function() {
    $("#pic1").hide();
    $("#pic2").show();                
});  
$("#pic2").click(function() {
    $("#pic2").hide();
    $("#pic3").show();                
});
$("#pic3").click(function() {
    $("#pic3").hide();
    $("#pic1").show();                
});
});

The rest is here. http://jsfiddle.net/XjdTX/3/

share|improve this question
    
You should include the main code in the question. –  ANeves Dec 10 '12 at 13:41

2 Answers 2

This should do the trick:

http://jsfiddle.net/XjdTX/11/

I changed the <div id="slideframe">...</div> to use a class instead. This will allow you to have multiple slide shows functioning off the same code, as shown in the jsFiddle. For toggling through the pics, the pic class is used and cycles based on the index of the clicked image.

There is also code in the jsFiddle that will fade the images instead of just toggling their visibility.

$(document).ready(function() {

    $('.slideframe').each(function() {

        var $pics = $(this).find('.pic'),
            max = $pics.length;

        $pics.on('click.slideframe', function(e) {
            var idx = $pics.index(this);
            if (idx < 0) {
                return false;
            }
            $(this).hide();
            $pics.eq(++idx % max).show();
        });
    });
});
share|improve this answer
3  
You should use $pics.on('click', handler) instead of $pics.click(handler). –  ANeves Dec 10 '12 at 13:44
    
@ANeves Thanks. –  tiffon Dec 10 '12 at 16:01
$(document).ready(function() {
    // set display:none for all <img> tags except the first
    $('.pic:gt(0)').hide();

    // stores all matches for class="pic"
    var $slides = $('.pic');

    $slides.click(function(){
        // stores the currently-visible slide
        var $current = $(this);    
        if( $current.is($slides.last()) ) {
            $current.hide();
            $slides.first().show();
        } 
        // else, hide current slide and show the next one
        else {
            $current.hide().next().show(); 
        }
    });
});

Edit Changed to include element caching, as per ANeves's suggestion.

Full code here: http://jsfiddle.net/XRpeA/

share|improve this answer
1  
You could suggest elements caching (e.g. $(this)). –  ANeves Dec 10 '12 at 16:13
    
@ANeves, I am about as far from being comfortable with JS as people come, so thanks for the suggestion. Did you mean something like this: jsfiddle.net/ta4YU/2 ? –  Ketchup1001 Dec 11 '12 at 1:16
1  
Precisely that! –  ANeves Dec 11 '12 at 11:02

Your Answer

 
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.