1
\$\begingroup\$

I needed to build a slider, and this is my first attempt. I'm trying to make it as dynamic as possible. My code works, but I'm wondering if there is a better way and if anyone has any suggestions. I'll be making the width calculated as well. I just have it hardcoded for testing.

Also, I need to make pagination for this. I'm not sure where to start with the way I have things set up. Any hints as to where to start would be much appreciated.

Here is the link to the working slider: http://wss.manage-website.com/stg/4406

Javascript:

jQuery.noConflict();
  jQuery(document).ready(function ($) {
    function doAnimation() {
      count = $('.slider img').length;
      $('.slider img').each(function(index) {
        // calculate and asign z-index to the images. First image in the stack recieves highest.
        var zindex = count - index;
        $(this).css('z-index', zindex);
        $(this).delay(3000*index).animate({
          left: '495px'
        }, 500, function() {
          $('.slider img').each(function(index) {
            // convert z-index to an integer
            var zindex = parseInt($(this).css('z-index'));
            // calculate z-index of images to retain order in the stack. Send this one to the back, push the rest up one.
            if (zindex < count) {
              $(this).css('z-index', zindex + 1);
            } else {
              $(this).css('z-index', zindex - (count-1));
            }
          });
          $(this).animate({
            left: '0'
          }, 500, function() {
            // if we're at the end, repeat.
            if (index == count - 1) {
              setTimeout(doAnimation,1500);
            }
          });
        });
      });
    }
    doAnimation();
  });
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Some minor improvements you can make:

  1. Cache your DOM lookups (e.g. $('.slider img')) to prevent having to parse the DOM multiple times. That's as simple as making a new variable:

    var sliderImages = $('.slider img');
    
  2. Use a ternary statement for setting your z-index:

    $(this).css('z-index', zindex < count ? zindex + 1 : zindex - count - 1);
    

    That'll remove 4 lines and remove unnecessary repetition.

  3. Add the var keyword to your count variable so it's not global (it should remain accessible to all the functions within your function scope anyway).

  4. Add some simple comments to explain why you're changing the z-index in the first place.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the tips! I've implemented them. Sorry about the no comments. The system at work strips comments from the code, so I'm very used to not putting them in! I've edited the code to include comments. \$\endgroup\$
    – Atomic Bop
    Commented May 25, 2012 at 2:36
  • \$\begingroup\$ OK, so now from your comments I finally understand what you're doing! You're not building a slider like I assumed, so much as as an image slider/carousel. \$\endgroup\$
    – Kit Grose
    Commented May 25, 2012 at 5:22
  • \$\begingroup\$ Right. Sorry for the confusion. Wasn't sure what to call it. \$\endgroup\$
    – Atomic Bop
    Commented May 25, 2012 at 13:02

Your Answer

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