patternjavascriptMinor
Custom slide show script
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:
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:
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.
The rest of the slideshow is contained within a separate parent element. There are four structures identical to this except that only one `.slide
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:
Is wrong, it should be something like
Of course, now you have the problem that the last slide will point to nothing..
Also, you have to check out your indenting, comments should be indented with the code, the code inside
Some more thinking in terms of arrays is needed here:
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:
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.