10
\$\begingroup\$

I decided to refactor the code for a responsive image gallery, that I setup not too long ago. Following the suggestions @joseph-the-dreamer made, and some other best practices, I cut down on the number of if statements.

Is there anything else I could do to make the activateSlider function more concise?

Is there another method one could use (in general) to avoid having to keep calling the same function on resize?

/* jQuery throttle / debounce - v1.1 - 3/7/2010 */ 
function debounce(a,b,c){var d;return function(){var e=this,f=arguments;clearTimeout(d),d=setTimeout(function(){d=null,c||a.apply(e,f)},b),c&&!d&&a.apply(e,f)}}

function activateSlider() {

   var slider = $('#slider'),
    sliderContainer = $('.orbit-container'),
    sliderContainerLinks = $(sliderContainer).children('a'),
    sliderKids = $('#slider li'),
    sliderContent = $('.slider-content'),
    sliderKidsContent = $(sliderKids).children(sliderContent),
    inactiveKidsContent = $('#slider li:not(.active) .slider-content'),
    accordionContainer = $('.accordion-container'),
    sliderBulletsContainer = $('.orbit-bullets-container');

  function transformSlider() {
    $(slider).foundation({
       orbit: {
         slide_number: false,
         timer: true,
         timer_speed: 8000,
         next_on_click: false
       }
   });

  if(window.matchMedia("(max-width: 767px)").matches) {
       /* Disable orbit gallery styles */
        $(sliderContainer).after($('.orbit-timer'));     
        $(slider, sliderKids).attr('style', '');
        $(slider).removeClass('orbit-slides-container').removeAttr('data-orbit').addClass('accordion-container');
        $(sliderKids).removeAttr('data-orbit-slide').removeClass('active');
        $(sliderContainerLinks, sliderKidsContent).hide();
        $(inactiveKidsContent).css('display', 'none');

      /* Init accordion click functions */
        $(sliderKids).unbind().bind('click', function(){
            $(this).toggleClass('active').siblings().removeAttr('class');
            $(this).siblings().find(sliderContent).slideUp();
          $(this).find(sliderContent).slideToggle();
        });     
  } else {       
    /* If accordion styles are present, remove them */
    var OrbitStyles = ($(accordionContainer).length === 0);
    if (!OrbitStyles) {
        $(sliderContainerLinks).show();
        $(slider).removeClass('accordion-container').addClass('orbit-slides-container').attr('data-orbit', '');
        $(sliderBulletsContainer).before($('.orbit-timer'));
        $('.orbit-timer').removeClass('paused');
    }
      $(sliderContent).show();
      $(sliderKids).first().addClass('active').siblings().removeAttr('class');       
  }      
}
 transformSlider();
}

$(function(){
  if (window.Foundation) {
    activateSlider();  
  }
});
/* Call the script on resize (must support ie8) */
if (window.addEventListener) {
   window.addEventListener('resize', debounce(function () {
    activateSlider();
  }, 250));
} else {
   window.attachEvent('resize', debounce(function () {
     activateSlider();
   }, 250));
 }

EDIT: It looks a little crude (stylewise) but the working version is here: http://jsbin.com/gicin/14/

\$\endgroup\$
  • \$\begingroup\$ Will you post a fiddle (or similar) of this so the intended functionality is explicit? \$\endgroup\$ – nrw May 30 '14 at 20:33
  • \$\begingroup\$ @nrw - Sure, I've added a link at the bottom to jsbin \$\endgroup\$ – Larz May 30 '14 at 21:00
  • \$\begingroup\$ Is there something you're wanting feedback on that's not in my answer? \$\endgroup\$ – nrw Jun 5 '14 at 6:04
  • \$\begingroup\$ @nrw - it took me a little while to carefully go through each step of your answer to be sure I understood, but it is definitely the kind of review I was looking for. Thank you! \$\endgroup\$ – Larz Jun 6 '14 at 13:20
3
\$\begingroup\$

First, I think you'd be better off if you had a representation of the gallery as data (following the Rule of Representation). It would go something like this:

A Simpler Method

Get your objects into memory. These can come from a REST API, or you could drop this into the dom when you're rendering server side. However you like.

var gallery = [
  {
    title: "An image",
    src: "http://placehold.it/1400x348",
    width: 1400,
    height: 348,
    body: 'More hence euphemistic...',
    active: true,
    index: 0
    // any property you'll need to render this item goes here
  },
  // etc...
]

You can have the rendering code that consumes this list. The example shown uses hyperscript.

var h = require('hyperscript')

function renderOne (slide) {
  h('li', {index: slide.index},
    h('img', {src: slide.src, width: slide.width}),
    h('section', slide.body)
    // and so on...
  )
}
function render (gallery) {
  h('ul#slider', gallery.map(renderOne))
}
// use it like this:
$('body').html(render(gallery))

Then you can listen for events on the containing element and modify the data accordingly.

$('body').on('click', 'li', function (e) {
  var index = parseInt($(e.currentTarget).attr('index'), 10)
  gallery.forEach(function (slide){
    slide.active = false
  })
  gallery[index].active = true
})

With every property necessary for rendering held in the data representation, your rendering logic can be very simple. Simple logic is easy to reason about and breeds fewer bugs. To get an accurate dom after any change, all you need to do is rerun:

$('body').html(render(gallery))

What about performance? There are excellent tools that abstract the dom into a virtual dom that determines the minimal change necessary to get the current dom to match the dom you want. Use those tools (like mercury or react).

If you're looking for another example, I did a similar post here.

The Original Method

That would be a significant change in method, however. Here's some ways to make your current method easier to reason about for a newcomer to your codebase (See it in action on jsbin here). Note: it was a style choice to omit semicolons and rely on automatic semicolon insertion.

Foundation now has a debounce function in its utils. May as well use that.

var debounce = window.Foundation.utils.debounce
var $copy, isSmall

You might also listen to requestAnimationFrame to get resize events. I left this as resize.

// Call the script on resize (must support ie8)
var listen = window.addEventListener ? window.addEventListener : window.attachEvent
listen('resize', debounce(checkGallery), 250)

The logic is easy to follow if methods are mostly in the order they're called.

$(function(){
  // when swapping widgets, clone original state
  $copy = $('#slider').clone()
  checkGallery()
})

Descriptive names for complex boolean questions tend to increase clarity.

function checkGallery () {
  var wasSmall = isSmall
  isSmall = window.matchMedia('(max-width: 767px)').matches

  // only swap widgets if necessary
  if (wasSmall !== isSmall) initGallery(isSmall)
}

function initGallery (small) {
  // reset to inital state
  $('#hero').html(clone())

  if (small) {
    initAccordion()
  } else {
    initOrbit()
  }
}

This retains the selected slide when swapping widgets. I believe this is not how the original worked, but I believe this was a desired behavior. I'd point out that this logic is hard to follow for a newcomer to your codebase. This function is a good example of why simple rendering logic + more complex data structure is the way to go.

function clone () {
  $html = $copy.clone()

  // retain active item
  index = $('#slider > li').index($('#slider > li.active'))
  console.log('index', index)
  if (index > -1) $html.find('> li:eq('+index+')').addClass('active')

  return $html
}

This way, we don't bring foundation JavaScript into the mix unless we're using it.

function initAccordion () {
  $('#slider').on('click', 'li', function (e) {
    $(e.currentTarget).addClass('active').siblings().removeClass('active')
  })
}

function initOrbit () {
  $('#slider').attr('data-orbit', '').foundation({
    orbit: {
      slide_number: false,
      timer: true,
      timer_speed: 8000,
      next_on_click: false
    }
  })
}

The only change necessary in CSS is this:

@media screen and (max-width: 767px) {
  #slider > li .slider-content {
    display: none;
  }
  #slider > li.active .slider-content {
    display: block;
  }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you very much for this thorough answer! Never even thought about listening for requestAnimationFrame instead of resize. Exactly the kind of answer I was looking for. \$\endgroup\$ – Larz Jun 6 '14 at 13:19
  • \$\begingroup\$ I'm glad to help! \$\endgroup\$ – nrw Jun 6 '14 at 16:36

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.