# Custom slide show script

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:

_____________________________________________________
|                                                   |
|                                                   |
|         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.

Its functionality is:

1. Starting from link 1, cycle through link 2, 3 and 4 and back to 1, infinitely, displaying each slide set for 45 seconds.
2. 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.

<ul class="navbar">

</ul>


The rest of the slideshow is contained within a separate parent element. There are four structures identical to this except that only one .slideset will have the class .current at any one time and each .slideset has an ID identical to it's corresponding links title and content.

I chose to keep each slide contained within its own parent element, that way, only the parent element needs to be changed.

<div class="slideset current" id="link1">

<div class="slideshow">

<div class="slidetext">

</div>

</div>

<ul class="slidedetails">

<li class="three">

<h2 class="valuetitle ctrtext">

Related Topic 1

</h2>

<p class="valuetext dt">

Lorem to da Ipsum

</p>

</li>

<li class="three">

<h2 class="valuetitle ctrtext">

Related Topic 2

</h2>

<p class="valuetext dt">

Lorem to da Ipsum

</p>

</li>

<li class="three">

<h2 class="valuetitle ctrtext">

Related Topic 3

</h2>

<p class="valuetext dt">

Lorem to da Ipsum

</p>

</li>

</div>


The 'Big Image' is applied as a background-image to the .slideshow div.

Now for the magic:

$(document).ready( //Set the frame rate of the slideshow and call the function setInterval(changeSlide, 45000) ); function changeSlide(){ /* remove the class that shows the current link as the active slide */$(".navlink").removeClass("selected");

/* create an array containing the names of each link */
/* find out which slide is currently showing and store it in currentSlide variable */
currentSlide = $(".slideset").closest(".current"); /* get the ID of the slide that's currently showing and store it in variable compare */ compare =$(currentSlide).attr("id");
/* compare the current slide with the array to find out which one it is, and then set the next slide target as a variable named nextSlide */
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
}
/* find the right slide based on number from if statement, set it as target variable */
target = allSlides[nextSlide]
/* add # to create a string of the ID of the target slide and set it as variable nextTarget */
nextTarget = ("#" + target)
/* define function for changing the slide */
slideTransition = function(){
/* Hide current slides */
$(".slideset").removeClass("current") .hide(); /* Show the next slides */$(nextTarget).fadeIn("fast")
};
/* create string of target link that needs to change */
var navchange = $(".navlink" + "[title=" + "'" + target + "'" + "]"); /* add highlight to new current link */$(navchange).addClass("selected");
/* change the slide! */
slideTransition()

}

$(".navbar").on("mouseenter" || "click", ".navlink", function (){ //change the current link to be front tab$(".navlink").removeClass("selected");
$(this).addClass("selected"); //Find the title of the link currently being hovered on var hoveredlink =$(this).attr("title");
//add the selector # and concatenate
var targetslides = ("#" + hoveredlink).toString();
//get the classes of the link in focus
var grabtarget = $(targetslides).attr("class"); //Parse to a string for comparison var isitONalready = grabtarget.toString(); console.log(isitONalready) //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; } //If it isn't visible else if (isitONalready !== itISonalready && isitONalready !== itISonalready2 && isitONalready !== itISonalready3){ //Fade out current slides$(".slideset").removeClass("current")
.hide();
//find the slides we want to show
correctslides = $(".slideset").closest(targetslides); //show the slides$(correctslides).addClass("current")
}

});


The part that actually changes the slide is as simple as adding the .current class. The .slideset class is set to display:none;, and the .current class has display:block; to override and show the element.

Can you suggest improvements, please? I'm sure this is not the most effective or beautiful way to achieve this.

Please comment to let me know if I've left anything out. I figure you don't need the CSS, but I can provide it if necessary.

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


• You are declaring a ton of variables with var <- very bad