7
\$\begingroup\$

I wrote the following code for Slick Slider, which changes the content below based on the current slide data-index. I just wonder if there is a more efficient way to do it?

http://codepen.io/pjmtokyo/pen/JYyjew

$('.slider').on('afterChange', function(event, slick, currentSlide, nextSlide){
    var elSlide = $(slick.$slides[currentSlide]);
    var dataIndex = elSlide.data('index');

  $('.content').each(function(){
    if ($(this).data('id') == dataIndex) {
      $(this).show();
    } else {
      $(this).hide();
    }  
  });

});
\$\endgroup\$

2 Answers 2

9
\$\begingroup\$

I would combine the two lines where you get the dataIndex, rename that to dataId to be consistent with the HTML element, and replace the loop by hiding everything, and then showing the correct element by finding it with a jQuery selector:

$('.slider').on('afterChange', function(event, slick, currentSlide, nextSlide){
    var dataId = $(slick.$slides[currentSlide]).data('index');  
    $('.content').hide();
    $('.content[data-id=' + dataId + ']').show();    
});

I also noticed that looking up dataId is superfluous, since it is simply the value of currentSlide plus one. So you could also make this change and retain the same behavior:

$('.slider').on('afterChange', function(event, slick, currentSlide, nextSlide){   
  $('.content').hide();
  $('.content[data-id=' + (currentSlide + 1) + ']').show();
});
\$\endgroup\$
5
  • \$\begingroup\$ Thanks, that's a great improvement Jonah. There was one thing that I didn't see yesterday – on page load all contents are visible (I only want to show content one on page load). To modify this I added the following CSS: .two, .three, .four, .five { display:none; } There is a slight odd behaviour that the page jumps up and down on slide change, unless this is just my screen height setup. Is CSS the way to go with this? \$\endgroup\$ Oct 11, 2015 at 23:43
  • \$\begingroup\$ I tried in chrome and safari on my mac, and didn't see any page jump. What browser are you using? \$\endgroup\$
    – Jonah
    Oct 12, 2015 at 0:05
  • \$\begingroup\$ oh, maybe it's just my screen height. I'm on a macbook using Chrome (Version 45.0.2454.101). What do you think about using CSS to hide the content? It works, but is it the best way. \$\endgroup\$ Oct 12, 2015 at 0:51
  • \$\begingroup\$ Another option would be to have only a single div for the content, and dynamically update its text, the same way you are dynamically updating the current slider. I think I'd prefer this approach. \$\endgroup\$
    – Jonah
    Oct 12, 2015 at 2:13
  • \$\begingroup\$ That sounds good, however the content will be dynamic and pulled in via other protocol. \$\endgroup\$ Oct 12, 2015 at 10:01
1
\$\begingroup\$

I can't see too many ways to improve this, but I have a few small changes you could make:

$('.slider').on('afterChange', function(event, slick, currentSlide, nextSlide){
    var elSlide = $(slick.$slides[currentSlide]);
    var dataIndex = elSlide.data('index');

  $('.content').each(function(){
    if ($(this).data('id') == dataIndex) {
      $(this).show();
    } else {
      $(this).hide();
    }  
  });

});

The show/hide loop can be simplified to a ternary statement:

  $('.content').each(function(){
    if ($(this).data('id') == dataIndex) {
      $(this).show();
    } else {
      $(this).hide();
    }  
  });

which would look like this:

  $('.content').each(function(){
    $(this).data('id') == dataIndex ? $(this).show() : $(this).hide();  
  });

The other change I could suggest is maybe removing the nextSlide parameter as it is superfluous and unused.

\$\endgroup\$
1
  • \$\begingroup\$ This is awesome, thanks Quill. Both your answers are very good and improved my code. There is one thing I missed though – on page load, all contents are visible (only content one should be visible). I added the CSS .two, .three, .four, .five { display:none; } There is a slight odd behaviour that the page jumps up and down on slide change, unless this is just my screen height setup. Can you think of a better way to do this? \$\endgroup\$ Oct 11, 2015 at 23:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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