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/
-
\$\begingroup\$ Will you post a fiddle (or similar) of this so the intended functionality is explicit? \$\endgroup\$nrw– nrw2014年05月30日 20:33:58 +00:00Commented May 30, 2014 at 20:33
-
\$\begingroup\$ @nrw - Sure, I've added a link at the bottom to jsbin \$\endgroup\$Larz– Larz2014年05月30日 21:00:50 +00:00Commented May 30, 2014 at 21:00
-
\$\begingroup\$ Is there something you're wanting feedback on that's not in my answer? \$\endgroup\$nrw– nrw2014年06月05日 06:04:38 +00:00Commented Jun 5, 2014 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– Larz2014年06月06日 13:20:06 +00:00Commented Jun 6, 2014 at 13:20
1 Answer 1
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;
}
}
-
1\$\begingroup\$ Thank you very much for this thorough answer! Never even thought about listening for
requestAnimationFrame
instead ofresize
. Exactly the kind of answer I was looking for. \$\endgroup\$Larz– Larz2014年06月06日 13:19:13 +00:00Commented Jun 6, 2014 at 13:19 -
\$\begingroup\$ I'm glad to help! \$\endgroup\$nrw– nrw2014年06月06日 16:36:49 +00:00Commented Jun 6, 2014 at 16:36