5
\$\begingroup\$

I'm new to JavaScript, and I've built a lightbox for an image gallery. There's a button to open the gallery, plus forwards, backwards, and close buttons, and keyboard controls. Currently, everything works exactly the way I intended, but I know that it can be more DRY, and would love some feedback on that.

Here is the relevant HTML for the image container:

<div class="gallery" data-image-source="img/gallery/project_main/1_IntegratedFarming.jpg">
 <button class="secondary">
 <i class="icon-gallery"></i>
 <span>View Gallery</span>
 </button> 
</div>

And this is the JavaScript:

var fillGalleryContainers = function() {
 $(".gallery").each(function(){
 $(this).css("background-image", "url('" + $(this).attr("data-image-source") + "')");
 });
};
var generateLightbox = function() {
 var $lightboxOverlayHTML = $('<div id="overlay"></div>');
 var $lightboxImageActiveHTML = $('<img class="gallery-image--active">');
 var $lightboxImageNextHTML = $('<img class="gallery-image--next">');
 var $logoPCLHTML = $('<i class="icon-pcl" id="logoPCL"></i>');
 var $logoCommunityHTML = $('<i class="icon-community-alt" id="logoCommunity"></i>');
 var $closeGalleryHTML = $('<i class="icon-close" id="closeGallery"></i>');
 var $navNextHTML = $('<i class="icon-arrow-right" id="navNext"></i>');
 var $navPrevHTML = $('<i class="icon-arrow-left" id="navPrev"></i>');
 $lightboxOverlayHTML.append(
 $lightboxImageActiveHTML,
 $lightboxImageNextHTML,
 $logoPCLHTML,
 $logoCommunityHTML,
 $navPrevHTML,
 $navNextHTML,
 $closeGalleryHTML
 );
 $('body').append($lightboxOverlayHTML);
};
var openLightbox = function() {
 $('.gallery button').click(function(event){
 var imageSource = $(this).parent().css("background-image");
 var $lightboxOverlay = $('#overlay');
 var $lightboxImageActive = $('.gallery-image--active');
 var $lightboxImageNext = $('.gallery-image--next');
 window.currentOverflow = $("html").css("overflow");
 imageSource = imageSource.replace('url(', '').replace(')','');
 $lightboxImageActive.attr("src", imageSource);
 $lightboxOverlay.css({
 top: $(window).scrollTop(),
 left: $(window).scrollLeft()
 });
 $lightboxOverlay.fadeIn(1000);
 $('html').css("overflow", "hidden");
 });
};
var closeLightBox = function() {
 $('#overlay').fadeOut(1000);
 $("html").css("overflow", window.currentOverflow);
};
var galleryNavigationNext = function() {
 var $activeImage = $('.gallery-image--active');
 var $nextImage = $('.gallery-image--next');
 var galleryImages = window.galleryImages;
 var galleryTotal = galleryImages.length;
 var location = window.location.href;
 var currentImageSource = $activeImage.attr('src');
 var modImageSource = currentImageSource.replace(location, '');
 var currentPosition = $.inArray(modImageSource, galleryImages);
 if (currentPosition < 0 ) {
 currentPosition = 0;
 }
 var next = (currentPosition + 1 < galleryImages.length - 1) ? currentPosition + 1 : 0;
 $nextImage.attr('src', galleryImages[next]);
 $activeImage.fadeOut(1000, function() {
 $activeImage.removeClass('gallery-image--active').addClass('remove');
 $nextImage.addClass('gallery-image--active').removeClass('gallery-image--next');
 $('.remove').remove();
 $('.gallery-image--active').after('<img class="gallery-image--next">');
 });
};
var galleryNavigationPrev = function() {
 var $activeImage = $('.gallery-image--active');
 var $nextImage = $('.gallery-image--next');
 var galleryImages = window.galleryImages;
 var galleryTotal = galleryImages.length;
 var location = window.location.href;
 var currentImageSource = $activeImage.attr('src');
 var modImageSource = currentImageSource.replace(location, '');
 var currentPosition = $.inArray(modImageSource, galleryImages);
 if (currentPosition < 0 ) {
 currentPosition = 0;
 }
 var next = (currentPosition - 1 >= 0) ? currentPosition - 1 : galleryImages.length - 1;
 $nextImage.attr('src', galleryImages[next]);
 $activeImage.fadeOut(1000, function() {
 $activeImage.removeClass('gallery-image--active').addClass('remove');
 $nextImage.addClass('gallery-image--active').removeClass('gallery-image--next');
 $('.remove').remove();
 $('.gallery-image--active').after('<img class="gallery-image--next">');
 });
};
var galleryEvents = function() {
 // Close Events
 $('#closeGallery').click(closeLightBox);
 $(document.documentElement).keyup(function(e){
 if (event.keyCode == 88 || event.keyCode == 27) {
 closeLightBox();
 }
 });
 // Open Events
 // Previous Events
 $('#navPrev').click(galleryNavigationPrev);
 $(document.documentElement).keyup(function(e){
 if (event.keyCode == 37) {
 galleryNavigationPrev();
 }
 });
 // Next Events 
 $('#navNext').click(galleryNavigationNext);
 $(document.documentElement).keyup(function(e){
 if (event.keyCode == 39 || event.keyCode == 32) {
 galleryNavigationNext();
 }
 });
};

Here is how the galleryImages array gets populated (which will be populated via Wordpress). It's just an inline script at the bottom of the page.

<script>
 window.galleryImages = [
 "img/gallery/project_main/1_IntegratedFarming.jpg",
 "img/gallery/project_main/2_BuildaCity.jpg",
 "img/gallery/project_main/3_LearningCenter.jpg",
 "img/gallery/project_main/4_CommonGrounds.jpg",
 "img/gallery/project_main/project_main_gallery01.jpg",
 "img/gallery/project_main/project_main_gallery02.jpg",
 "img/gallery/project_main/project_main_gallery03.jpg",
 "img/gallery/project_main/project_main_gallery04.jpg",
 "img/gallery/project_main/project_main_gallery05.jpg",
 "img/gallery/project_main/project_main_gallery06.jpg",
 "img/gallery/project_main/project_main_gallery07.jpg",
 "img/gallery/project_main/project_main_gallery08.jpg",
 "img/gallery/project_main/project_main_gallery09.jpg",
 ];
</script>

I know that the galleryNavigationNext and previous functions can be combined, but I'm not quite sure how passing variables from functional calls works. I've tried the below and it didn't work:

var galleryNavigation = function(direction) {
 var $activeImage = $('.gallery-image--active');
 var $nextImage = $('.gallery-image--next');
 var galleryImages = window.galleryImages;
 var galleryTotal = galleryImages.length;
 var location = window.location.href;
 var currentImageSource = $activeImage.attr('src');
 var modImageSource = currentImageSource.replace(location, '');
 var currentPosition = $.inArray(modImageSource, galleryImages);
 if (currentPosition < 0 ) {
 currentPosition = 0;
 }
 var next;
 if (direction === "next") {
 next = (currentPosition + 1 < galleryImages.length - 1) ? currentPosition + 1 : 0;
 } else if (direction === "prev") {
 next = (currentPosition - 1 >= 0) ? currentPosition - 1 : galleryImages.length - 1;
 }
 $nextImage.attr('src', galleryImages[next]);
 $activeImage.fadeOut(1000, function() {
 $activeImage.removeClass('gallery-image--active').addClass('remove');
 $nextImage.addClass('gallery-image--active').removeClass('gallery-image--next');
 $('.remove').remove();
 $('.gallery-image--active').after('<img class="gallery-image--next">');
 });
};
var galleryEvents = function() {
 $('#navPrev').click(galleryNavigation("prev"));
 $(document.documentElement).keyup(function(e){
 if (event.keyCode == 37) {
 galleryNavigation("prev");
 }
 });
 $('#navNext').click(galleryNavigation("next"));
 $(document.documentElement).keyup(function(e){
 if (event.keyCode == 39 || event.keyCode == 32) {
 galleryNavigation("next");
 }
 });
};
Alex L
5,7832 gold badges26 silver badges69 bronze badges
asked Jul 10, 2014 at 17:37
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I'll be reviewing your first chunk of code.

fillGalleryContainers

  • In the future, you'll probably† be able to just write:

    $(".gallery").css("background-image", "url(attr(data-image-source url))");

  • For now, instead of .attr("data-image-source"), you can use .data("image-source").

  • You may want to save your $(this) object into a variable. Creating jQuery objects is not free.

generateLightbox

  • This is cute, but I'm not sure that there's really any value in creating a jQuery object for each line of HTML; it's definitely less performant than just mashing it all into one string.

    $lightboxOverlay =
 $('<div id="overlay">' +
 ' <img class="gallery-image--active" />' +
 [...]
 '</div>');

    The rest of the points are independent of this one.

  • It doesn't make much sense to me to name the variables $...HTML. It doesn't actually represent the HTML (that would be just the string); instead, it's a jQuery object, which is made clear by the $ prefix. I'd prefer, e.g. var $navPrev = ...;.

  • I really do not like the use of <i> here. This is semantic garbage, to be blunt. You could be using <button> or <a>, or even <img>, but using <i> is bad.

  • As a matter of taste, you could choose to use $lightboxOverlay.appendTo('body'); instead of $('body').append($lightboxOverlay);. This would allow you to just chain the method on.

openLightbox

  • This is the first function where you've been inconsistent in quotes. I can't really tell if you prefer " or ', but you should stick with one or the other. You can make exceptions for when the string will contain a lot of the same type of quote; for example, I like all my strings to use double-quotes, but when I'm writing a string with HTML, I use single-quotes, like you did in generateLightbox.

  • Prefer .on('click', function... over .click(function....

  • You aren't using event, so no need to put it in. Just go with function() {....

  • $lightboxActiveImage and $lightboxNextImage sound a bit better to me, but it's a matter of taste.

  • In fact, you don't even use $lightboxImageNext, so just get rid of it.

  • I would prefer the following, in case your background-image url for some reason has ) in it. It reads more cleanly to me too, but that might just be me.

    imageSource = imageSource.match(/^url\('(.+)'\)$/)[1];

  • I notice that you're storing the current value of overflow on your <html> element. This isn't strictly necessary: unless the <html> tag actually has a style='overflow: scroll;' on it, you can just set it to empty string later:

    $("html").css("overflow", "hidden");
// later, in closeLightbox
$("html").css("overflow", "");

  • Take advantage of method chaining: $lightboxOverlay.css().fadeIn();.

  • If you can deal with openLightbox always resetting, then instead of using imageSource, you could just do:

    $lightboxImageActive.attr("src", $(this).parent().data("image-source"));

  • Frankly, I don't like that the openLightbox function actually just assigns an event handler. I'd rather do that in the galleryEvents function.

closeLightBox

  • No. Bad. This name is inconsistent with the rest of your code. Rename it to closeLightbox.

  • As stated before, just do $("html").css("overflow", "");.

galleryNavigationNext and galleryNavigationPrev

You're right; these two functions share a lot of code. I propose a single function, which I'll call galleryNavigateRelative. I'll add that to the end of this section, but first, the line-by-line analysis:

  • You never use galleryTotal. Take it out.

  • I notice that you try to "unresolve" the image src. I think a better solution would be to resolve and normalise window.galleryImages when loading. If you use the URI.js library, you can resolve like so:

    window.galleryImages = window.galleryImages.map(function(val) { return URI(val).absoluteTo().toString(); });

    This will also deal with any pesky accidental //s. (A note of caution: if your string starts with //, then this will not behave quite as you expect.)

  • The assignment of currentPosition could be simplified:

    var currentPosition = Math.max(0, $.inArray(modImageSource, galleryImages));

  • Your calculation of next seems like a perfect use case for the modulus operator %:

    var next = (currentPosition + 1) % galleryImages.length;

  • The logic in the callback for fadeOut is pretty complicated and could be simplified. You're immediately removing $activeImage, so why bother with the classes? Also, why do a new search for .gallery-image--active when you already have it as $nextImage?

    $activeImage.fadeOut(1000, function() {
 $activeImage.remove();
 $nextImage.toggleClass("gallery-image--active gallery-image--next")
 .after('<img class="gallery-image--next" />');
});

  • Here's what you'll need to change to DRY it up:

    var galleryNavigateRelative = function(delta) {
 ...
 var next = (currentPosition + delta) % galleryImages.length;
 ...
}

galleryEvents

  • I don't like the function name. Change it to something like bindGalleryEvents or galleryBindEvents, if you want to stick with the pseudo-namespacing. You need a verb of some sort in there.

  • Again, prefer .on("click", ... and .on("keyup", ....

  • In your case, there's no difference‡ between assigning a keyup handler to $(document), so use that for brevity.

  • jQuery normalises event.which, so use that over .keyCode. (I think .keyCode might work too, but I believe it's typical practice to use which.)

  • If you're passing e to your callback, then why use the global event in the body?

  • You may wish to aggregate your keyup handlers.

  • In your keyup handlers, consider commenting what the keyCode represents.

  • As mentioned before, it's better to bind the openLightbox function here.

  • With our galleryNavigateRelative function, we'll need to do something slightly different when binding to #navPrev and #navNext:

    $("#navPrev").on("click", function() { galleryNavigateRelative(-1); });
// and similar for next

Here's my final reviewed version of your code:

var fillGalleryContainers = function() {
 $(".gallery").each(function(){
 var $this = $(this);
 $this.css("background-image", "url('" + $this.data("image-source") + "')");
 });
};
var generateLightbox = function() {
 $("body").append(
 '<div id="overlay">' +
 ' <img class="gallery-image--active" />' +
 ' <img class="gallery-image--next" />' +
 ' <a class="icon-pcl" id="logoPCL"></a>' +
 ' <button class="icon-community-alt" id="logoCommunity"></button>' +
 ' <button class="icon-close" id="closeGallery"></button>' +
 ' <button class="icon-arrow-right" id="navNext"></button>' +
 ' <button class="icon-arrow-left" id="navPrev"></button>' +
 '</div>'
 );
};
var openLightbox = function() {
 // `this' is a `.gallery button'
 var $lightboxOverlay = $("#overlay");
 var $lightboxActiveImage = $(".gallery-image--active");
 $lightboxImageActive.attr("src", $(this).parent().data("image-source"));
 $lightboxOverlay.css({
 top: $(window).scrollTop(),
 left: $(window).scrollLeft()
 }).fadeIn(1000);
 $("html").css("overflow", "hidden");
};
var closeLightbox = function() {
 $("#overlay").fadeOut(1000);
 $("html").css("overflow", "");
};
var galleryNavigateRelative = function(delta) {
 var $activeImage = $(".gallery-image--active");
 var $nextImage = $(".gallery-image--next");
 var galleryImages = window.galleryImages;
 var location = window.location.href;
 var currentImageSource = $activeImage.attr("src");
 var modImageSource = currentImageSource.replace(location, "");
 var currentPosition = $.inArray(modImageSource, galleryImages);
 if (currentPosition < 0 ) {
 currentPosition = 0;
 }
 var next = (currentPosition + delta) % galleryImages.length;
 $nextImage.attr("src", galleryImages[next]);
 $activeImage.fadeOut(1000, function() {
 $activeImage.remove();
 $nextImage.toggleClass("gallery-image--active gallery-image--next")
 .after('<img class="gallery-image--next" />');
 });
};
var galleryEvents = function() {
 // open/close
 $("#closeGallery").on("click", closeLightbox);
 $(".gallery button").on("click", openLightbox);
 // navigation
 $("#navPrev").click(function() {
 galleryNavigateRelative(-1);
 });
 $("#navNext").click(function() {
 galleryNavigateRelative(+1);
 });
 // keyboard handling
 $(document).on("keyup", function(e) {
 if (e.which == 88 || e.which == 27) {
 // "x" or ESC
 closeLightbox();
 } else if (e.which == 37) {
 // left arrow
 galleryNavigateRelative(-1);
 } else if (e.which == 39 || e.which == 32) {
 // right arrow or space
 galleryNavigateRelative(+1);
 }
 });
};

† I say "probably" because although it's a CSS3 Candidate Recommendation, it's considered at-risk due to poor browser support.
‡ Strictly, your code will be assigning the keyup handler to $("html"), but they'll do the same thing.

answered Jul 30, 2014 at 1:02
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your feedback. You really did a thorough job, and its all very relevant. Thanks! One follow up question, I've asked around a bit and haven't gotten a great answer yet, but is there a reason that on.('click') is preferred over click('')? Same goes for any event, really. Thanks! \$\endgroup\$ Commented Jul 31, 2014 at 1:28
  • \$\begingroup\$ @idealbrandon I guess it doesn't really matter. .click() is the shorthand. It adds a very, very slight overhead (a conditional check and an additional function call), but this is negligible. I've always used .on out of habit because of how versatile it is (if you want to handle multiple events, you can do .on("click mouseleave", ...) or something) and because it allows me to easily convert an event binding to a delegated event (.on("click", ".added-by-ajax", ...)). \$\endgroup\$ Commented Jul 31, 2014 at 4:55

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.