javascriptjqueryslideshowjquery-click-event

Jquery function isn't adding class to Dom elements when it should be. Trying to build a button function slideshow


I'm trying to create a slideshow for a project.

The code I've built is fairly simple but for some reason I can only make the next arrows work, not the previous.

When you click next it removes the class active from the current image, adds that class to the next image and changes the display to the new active image.

When you click previous it removes the active class but doesn't add any classes to any other elements. It then changes the display to the final image for some reason.

I've tried rearranging the order that the functions are called in. I've also tried to see what happens when the active class is added to the last image (it has the same problem but in reverse, so now next doesn't work but previous is fine).

var activeSlide = $('.active');
var nextSlide = activeSlide.next('.slide');
var prevSlide = activeSlide.prev('.slide');
var arrow = $('.arrow');
var firstSlide = $('.first');
var lastSlide = $('.last');

// slideshow functions

// next arrow
function nextArrow () {
  if (!activeSlide.hasClass('last')) {
            activeSlide.removeClass('active');
            nextSlide.addClass('active');
            activeSlide = nextSlide;
            nextSlide = activeSlide.next('.slide');
            // check if new slide is last slide and remove the click button
            if (activeSlide.hasClass('last')) {
                $('.next').fadeOut();   
            }
        }  
};
// previous arrow
function prevArrow () {
    if(!activeSlide.hasClass('first')) {
        activeSlide.removeClass('active');
        prevSlide.addClass('active');
        activeSlide = prevSlide;
        prevSlide = activeSlide.prev('.slide');
        // check if new slide is the first slide and remove the click button
        if (activeSlide.hasClass('first')) {
            $('.previous').fadeOut();
        }
    } 
};

//click element and call functions
$('.arrow').click( function () {
    if ($(this).hasClass('next')) {
        nextArrow();
    } else {
        prevArrow();        
    }
});
#slideshow {
    position: relative;
    height: 400px;  
    width: 600px;
    margin: 15% auto;
}
.slide {
    position: absolute;
    top: 0;
    left: 0;
    z-index: 8;
    height: 100%;
    width: 100%;
}
.active {
    z-index: 10;
}
.arrow {
    text-decoration: none;
    display: inline-block;
    padding: 8px 16px;
}
.arrow:hover {
    background-color: #ddd;
    color: black;
    opacity: 0.9;
    cursor: pointer;
}
.previous {
    left: 0;
}

.next {
    right: 0;
}
.arrow {
    z-index: 11;
    background-color: #ddd;
    color: black;
    position: absolute;
    top: 50%;
    opacity: 0.5;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<div id="slideshow">
        <div id="slides">
            <img class="slide active first" src="Toscana 1.jpg" alt="image of toscana, slideshow image 1" />
            <img class="slide" src="Toscana 2.jpg" alt="image of toscana, slideshow image 1" />
            <img class="slide" src="Toscana 3.jpg" alt="image of toscana, slideshow image 2" />
            <img class="slide last" src="Toscana 4.jpg" alt="image of toscana, slideshow image 3" />
        </div>
        <div class="previous arrow">&#8249;</div>
        <div class="next arrow">&#8250;</div>
    </div>
    <script src="javascript/practice.js"></script>

In theory you should be able to click next and see the next image in the sequence. Then when you want to see the previous image in the sequence you should be able to click previous whereby it will remove the active class from the current image and add it to the image before.

Really appreciate the help understanding what's going wrong if anyone is able to help at all.


Solution

  • You're depending on global variables to maintain your program state, but aren't always managing to keep the state of the variables matching up with the state of the DOM.

    It's probably better to use the DOM as a single source of truth: the "active" slide is whichever node currently has the "active" class, and you can calculate the next and previous ones based on its position.

    This also needed some work on the "next" / "previous" button visibility: you were hiding buttons at the end and beginning of the range, but never brought them back; you also need to load with the "previous" button disabled initially.

    //No global variables. Use $('.active') to determine the currently active slide
    
    // next arrow
    function nextArrow() {
      var activeSlide = $('.active');
      if (activeSlide.hasClass('last')) return; // bail if the user managed to click the 'next' button while it was fading out
    
      activeSlide.removeClass('active');
      activeSlide.next().addClass('active');
      // $('.active') is now the "next" slide
      if ($('.active').hasClass('last')) { 
        $('.next').fadeOut();
      } 
      
      // Always bring back the "previous" button when user hits next:
      $('.previous').fadeIn(); 
    };
    
    // previous arrow
    function prevArrow() {
      var activeSlide = $('.active');
      if (activeSlide.hasClass('first')) return;
      activeSlide.removeClass('active');
      activeSlide.prev().addClass('active');
      if ($('.active').hasClass('first')) {
        $('.previous').fadeOut();
      }
      $('.next').fadeIn();
    };
    
    //click element and call functions
    // removing the unnecessary intermediate function here:
    $('.next').click(nextArrow)
    $('.previous').click(prevArrow)
    #slideshow {
      position: relative;
      height: 200px;
      width: 300px;
      margin: 15% auto;
    }
    
    .slide {
      position: absolute;
      top: 0;
      left: 0;
      z-index: 8;
      height: 100%;
      width: 100%;
    }
    
    .active {
      z-index: 10;
    }
    
    .arrow {
      text-decoration: none;
      display: inline-block;
      padding: 8px 16px;
    }
    
    .arrow:hover {
      background-color: #ddd;
      color: black;
      opacity: 0.9;
      cursor: pointer;
    }
    
    .previous {
      left: 0;
      display:none;
    }
    
    .next {
      right: 0;
    }
    
    .arrow {
      z-index: 11;
      background-color: #ddd;
      color: black;
      position: absolute;
      top: 50%;
      opacity: 0.5;
    }
    <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
    <div id="slideshow">
      <div id="slides">
        <img class="slide active first" src="https://placehold.it/600x400" alt="image of toscana, slideshow image 1" />
        <img class="slide" src="https://placehold.it/601x401" alt="image of toscana, slideshow image 2" />
        <img class="slide" src="https://placehold.it/602x402" alt="image of toscana, slideshow image 1" />
        <img class="slide last" src="https://placehold.it/603x403" alt="image of toscana, slideshow image 1" />
      </div>
      <div class="previous arrow">&#8249;</div>
      <div class="next arrow">&#8250;</div>
    </div>