HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Custom slide show script

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scriptcustomslideshow

Problem

I've built a basic image slide show with jQuery, nothing particularly fancy, but there a few specific things I wanted it to do and I managed it - I just don't think I did it the best way possible. Hoping to get some pointers on where I can improve it.

So the basic visual structure is:

_____________________________________________________
|___Link 1___|___Link 2___|___Link 3___|___Link 4___|
|                                                   |
|                                                   |
|         Big Image relating to active link         |
|                                ___________________|
|                               |    Little text    |
|_______________________________|___box overlayed___|
   ______________   ______________   _____________
   |  Related   |   |   RT2      |   |   RT3     |
   |   topic 1  |   |            |   |           |
   |____________|   |____________|   |___________|


There are four 'Big Images', and 4 sets of related topics (4 x 3 related topics). Each image and set of related topics are attached to one of the links. So, attached to each link, is a 'slide set' containing one image, text overlay, and three related topics.

Also note that the links are the sites main navigation.

Its functionality is:

  • Starting from link 1, cycle through link 2, 3 and 4 and back to 1, infinitely, displaying each slide set for 45 seconds.



  • If the user hovers over a link, change to it's corresponding slide set, and continue the slideshow from there.



So now that you now what it looks like and what it's supposed to do, let me explain how I've structured it in my HTML:

I've kept the main navigation links completely separate from the rest of the slideshow, to ensure it displays even if there's a browser issue ruining the slideshow.


    Link 1

    Link 2

    Link 3

    Link 4


The rest of the slideshow is contained within a separate parent element. There are four structures identical to this except that only one `.slide

Solution

You have to think more in terms of arrays

This:

if(compare === allSlides[0])
    {
        nextSlide = 1
    }
    else if(compare === allSlides[1])
    {
        nextSlide = 2
    }
    else if(compare === allSlides[2])
    {
        nextSlide = 3
    }
    else if(compare === allSlides[3])
    {
        nextSlide = 0
    }


Is wrong, it should be something like

for( var i = 0 ; i < allSlides.length ; i++ )
{
  if( compare === allSlides[i] )
  {
    nextSlide = i + 1;
  }
}


Of course, now you have the problem that the last slide will point to nothing..

for( var i = 0 ; i < allSlides.length ; i++ )
{
  if( compare === allSlides[i] )
  {
    nextSlide = ( i == allSlides.length - 1 ) ? 0 : i+1;
  }
}


Also, you have to check out your indenting, comments should be indented with the code, the code inside slideTransition should be indented as well. Now that I read your code again, there seems to be no point at all in capturing those 2 statements in slideTransition, just execute them in the main function.

Some more thinking in terms of arrays is needed here:

//Possible variations returned as grabtarget
itISonalready = "slideset current"
itISonalready2 = "slideset .current"
itISonalready3 = ".slideset .current"

//If it's already visible
if (isitONalready === itISonalready || isitONalready === itISonalready2 || isitONalready === itISonalready3){
    console.log("It is on already")
    return;
}


I would hate to see this code once you have 7 slides ;P

Finally, there are 2 more items that jshint.com found for me:

  • You are inconsistent in applying semicolons



  • You are declaring a ton of variables with var

Code Snippets

if(compare === allSlides[0])
    {
        nextSlide = 1
    }
    else if(compare === allSlides[1])
    {
        nextSlide = 2
    }
    else if(compare === allSlides[2])
    {
        nextSlide = 3
    }
    else if(compare === allSlides[3])
    {
        nextSlide = 0
    }
for( var i = 0 ; i < allSlides.length ; i++ )
{
  if( compare === allSlides[i] )
  {
    nextSlide = i + 1;
  }
}
for( var i = 0 ; i < allSlides.length ; i++ )
{
  if( compare === allSlides[i] )
  {
    nextSlide = ( i == allSlides.length - 1 ) ? 0 : i+1;
  }
}
//Possible variations returned as grabtarget
itISonalready = "slideset current"
itISonalready2 = "slideset .current"
itISonalready3 = ".slideset .current"

//If it's already visible
if (isitONalready === itISonalready || isitONalready === itISonalready2 || isitONalready === itISonalready3){
    console.log("It is on already")
    return;
}

Context

StackExchange Code Review Q#28391, answer score: 2

Revisions (0)

No revisions yet.