3
\$\begingroup\$

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:

  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">

    <li class="navlink selected" title="link1">Link 1</li>

    <li class="navlink"  title="link2">Link 2</li>

    <li class="navlink" title="link3">Link 3</li>

    <li class="navlink" title="link4">Link 4</li>

</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 */
    allSlides = ["link1", "link2", "link3", "link4"]
    /* 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")
            .addClass("current");
        };
/* 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")
                        .fadeIn("fast");
    }

});

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.

To view it in action click here.

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.

\$\endgroup\$
2
\$\begingroup\$

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 <- very bad
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.