4
\$\begingroup\$

I built a testimonial slider that automatically plays on load and stops auto playing when the slider controls are used to change the active slide.

I feel like it could be optimized easily, especially the adding and removing active class seems like it could be made better but I'm not sure how.

$(document).ready(function(){ // Check var to see if slider controls has been clicked var clickedSlider = false; // Automatically change the active Testimonial Slide function autoSlide(){ // Get active Slide en corresponding Control item var activeSlide = $('.testimonial-slider-slides a.active'); var activeControl = $('.testimonial-slider-controls a.active'); // If last testimonial is active start back from the beginning if(activeSlide.is(':last-child')){ activeSlide.removeClass('active'); activeControl.removeClass('active'); $('.testimonial-slider-slides a').first().addClass('active'); $('.testimonial-slider-controls a').first().addClass('active'); } else { // Else go to next slide activeSlide.removeClass('active'); activeControl.removeClass('active'); activeSlide.next().addClass('active'); activeControl.next().addClass('active'); } } // If controls haven't been clicked, autoplay the slider window.setInterval(function(){ if (clickedSlider === false) { autoSlide(); } }, 2000); // Show corresponding slide when clicking on slider control $('.testimonial-slider-controls a').click(function(e){ e.preventDefault(); clickedSlider = true; $('.testimonial-slider-controls a.active').removeClass('active'); $(this).addClass('active'); var slide = ($(this).data('for')); $('.testimonial-slider-slides a').removeClass('active'); $('[data-id="' + slide + '"]').addClass('active'); }); });

Any thoughts on this?

Update: This is the HTML I use:

<div class="testimonial-slider"> <div class="testimonial-slider-slides"> <div class="testimonial-slider-slides-slide active" data-id="1"> <!-- Slide content --> </div> <!-- extra slides --> </div> <div class="testimonial-slider-controls"> <a href="#" data-for="1" class="active">Slide 1</a> <a href="#" data-for="2">Slide 2</a> <a href="#" data-for="3">Slide 3</a> </div> </div>

This was updated after i first posted here but the only real change in the javascript / jquery is the slides are now selected by .testimonial-slider-slides-slide instead of .testimonial-slider-slides a

Thanks to Mike Brant's Answer the data-for and data-id attributes can be left out

\$\endgroup\$
1
  • \$\begingroup\$ Can you show corresponding HTML? \$\endgroup\$ Commented Jun 7, 2017 at 14:39

1 Answer 1

Reset to default
3
\$\begingroup\$

Some thoughts:

  • Don't continually re-query the DOM. Cache your control and slide elements in a variable so you don't need to spend time re-querying for them.
  • You don't need to maintain state around clickedSlider condition. Consider using clearInterval() instead as more "proper" way to manage interval functionality.
  • Consider breaking apart your autoSlide() function into it's key parts - one function for managing the current slide index and another for actually performing the action of making slide at given index "active". This gives you more code re-use in giving the capability to jump directly to a slide at a given index as is needed in your control click handler.
  • Do you really need data properties for your controls and slides? If there is a 1-to-1 relationship between the slides and the controls and their ordering in DOM is the same, you should be able to know the control at index X relates to the slide at index X.

This might give you code more like:

$(document).ready(function(){ var $testimonialSlides = $('.testimonial-slider-slides a'); var $testimonialControls = $('.testimonial-slider-controls a'); var slideCount = $testimonialSlides.length; var slideIndex = 0; function activateSlide(idx) { $testimonialSlides .removeClass('active') .eq(idx).addClass('active'); $testimonialControls .removeClass('active') .eq(idx).addClass('active'); } function autoSlide(){ slideIndex++; if (slideIndex === slideCount) { slideIndex = 0; } activateSlide(slideIndex); } var autoSlideInterval = window.setInterval(autoSlide, 2000); $testimonialControls.click(function(e){ e.preventDefault(); window.clearInterval(autoSlideInterval); slideIndex = $testimonialControls.index(this); activateSlide(slideIndex); }); });

You may also consider building this as a proper jQuery plug-in or class (though not shown in my example above). This would allow you to better encapsulate state of the slider functionality, such that state would not need to be held in main $(document).ready() scope. This also potentially makes this simple functionality re-usable in the future - just pass in config for DOM selectors, class name(s) to apply (i.e. active), interval time, etc. I would rather see a usage pattern like:

$(document).ready(function() { // Perhaps not all of the configurations shown below need to be passed // if there are default settings. // This assumes autoslider() code is already included in document $('.testimonial-slider-slides a').autoslider( { controls: '.testimonial-slider-controls a', activeSlideClass: 'active', interval: 2000, startIndex: 0 } ); // other application code that needs to execute on ready() // perhaps even another autoslider with it's own encapsulated state });

Why have superfluous comments? If your code is well-written with meaningful function and variable names (I think you do a good job of this), then you should not need such comments.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you, not only does this work perfectly, I also learned quite a bit from it. I had to make a few small changes because the HTML structure changed a bit. I'm going to make it into a plugin like you said, so I can easily reuse it. Again, thank you ! \$\endgroup\$ Commented Jun 8, 2017 at 9:01

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.