5
\$\begingroup\$

I'm using jQuery to swap images on hover. There's a main image, and a gallery of thumbs. When the thumb is hovered over/touched, the larger version of it populates the main image, and the smaller version of the original populates the thumb. This happens via swapping src and data- attributes.

This all works, but being relatively new to JS/jQuery, the code is probably a little clunky and could use a keener eye. I don't fully grasp callbacks (among other JS things) yet, so I imagine there'll be some streamlining/chuckling with that.

function large_to_thumb(imageHovered, mainLarge, mainThumb){
    $(imageHovered).attr('src', mainThumb).data('large', mainLarge);
}

function thumb_to_large(image, mainLarge, mainThumb){
    var mainImage = $('.main-image');
    // 1. put gallery image data-large into .main-large src
    mainImage.attr('src', $(image).data('large'));

    // 2. put gallery image src into .main-large data-gallery-image
    mainImage.data('gallery-image', $(image).attr('src'));
    large_to_thumb(image, mainLarge, mainThumb);
}


$('.product-gallery img').on('mouseenter touch', function(){
    var mainLarge = $('.product-image .main-image').attr('src');
    var mainThumb = $('.product-image .main-image').data('gallery-image');
    thumb_to_large(this, mainLarge, mainThumb);
});
li {
  display:inline;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div class="product-image">
    <img class="main-image" src="http://placehold.it/300x300" data-gallery-image="http://placehold.it/150x150">
    <div class="product-gallery">
      <ul>
        <li>
          <img src="http://placehold.it/149x149" data-large="http://placehold.it/299x299">
        </li>
        <li>
          <img src="http://placehold.it/148x148" data-large="http://placehold.it/298x298">
        </li>
      </ul>
    </div>
  </div>

\$\endgroup\$
  • \$\begingroup\$ You can give the placehold.it images different colors by adding a "slash hexadecimal-color value" at the end of the URL. See this example: ' src="placehold.it/300x300/00f" ' . These example provides a square with blue background. So you can differentiate the images. \$\endgroup\$ – michael.zech Aug 31 '16 at 9:26
  • \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$ – Mast Aug 31 '16 at 12:23
  • \$\begingroup\$ thanks @Mast, I only changed the colour of images so they are easier to differentiate, which may make it easier to see the result (and the changes weren't because of an answer, but a comment on the question). No functionality was changed, no additional lines, just file names. If that's still too much, no problem. \$\endgroup\$ – alexwc_ Aug 31 '16 at 14:00
  • 1
    \$\begingroup\$ It's covered in Mike's answer, so you'll be invalidating his answer by changing it. We take that seriously. Feel free to post a follow-up question with the revised code instead. \$\endgroup\$ – Mast Aug 31 '16 at 14:04
3
\$\begingroup\$

Some good suggestions already from @konijn, so I won't rehash any of the content from that answer.

I would consider writing this a a jQuery plugin. This will allow you to properly namespace your code and allow you to define your own variables for re-use within this scope (i.e. to prevent need to re-query DOM with every event action. This will also make your solution re-usable across various websites and DOM configurations.

That might look like

(function( $ ) {

    $.fn.galleryThumbSwapper = function(options) {

        // Extend our default options with those provided.
        // Note that the first argument to extend is an empty
        // object – this is to keep from overriding our "defaults" object.
        var opts = $extend({},$.fn.galleryThumbSwapper.defaults, options);

        // do our DOM selections once
        // define wrapper based on element this function was called on
        var $wrapper = $(this);

        // define main image and gallery image items
        // here we only find element within the wrapper such that
        // you could use multiple galleries on a page without
        // worry about interaction problems between galleries so
        // long as each gallery is contained within the wrapper
        var $mainImage = $wrapper.find(opts.mainImageSelector);
        var $galleryImages = $wrapper.find(opts.galleryImageSelector);

        // set click handler
        $galleryImages.on('click', swapImages);

        // define callback for click handler
        function swapImages() {
            // wrapper for triggered element
            $triggerElement = $(this);

            // get current main image info
            var currentMainSrc = $mainImage.attr('src');
            var currentMainData = $mainImage.data('gallery-image');

            // get triggered thumbnail info
            var newMainSrc = $triggerElement.data('large');
            var newMainData = $triggerElement.attr('src');

            // perform the swap
            $mainImage.attr('src', newMainSrc);
            $mainImage.data('gallery-image', newMainData);

            $triggerElement.attr('src', currentMainSrc);
            $triggerElement.data('large', currentMainData);
        }

        // return this for use in chaining
        return this;
    }

    // define default values for plug-in
    // you could also possibly add default values for the data fields here
    // so as to make these configurable
    $.fn.galleryThumbSwapper.defaults = {
        mainImageSelector = '.main-class',        
        galleryImageSelector = '.product-gallery img'
    }; 
}( jQuery ));


// usage
$(document).ready(
    $('.product-image').galleryThumbSwapper();
);

// or, with overriding default options
$(document).ready(
    $('.product-image').galleryThumbSwapper({
        mainImageSelector = '#main_img',        
        galleryImageSelector = '.thumb_img'            
    });
);

A few other thoughts:

  • You might consider using id instead of or in addition to class for the main wrapper DOM element. i.e. product-image. This would guarantee uniqueness, particularly if you used the jQuery plug-in approach I am suggesting.
  • You might consider your data attribute naming of 'large' which doesn't seem to make much sense as what you are storing in there is a URL. I actually don't see why you can't use the same data-attribute name on the main image and the gallery images.
  • Seems like a bit of an odd user experience to have thumbnails potentially be changed in order based on how the user triggers these image swaps. To me as a user, I would expect to be able to hover across the thumbs, without it impacting the order of the thumbnails.
  • Consider having images have their own class as well. This would address current inconsistency in having some elements be included in logic based on class while others become part of the logic based on element type.
\$\endgroup\$
  • \$\begingroup\$ this looks interesting - I'll be trying it out. One thing: is there a character missing on the var opts = ... line? There's a parenthesis at the end that doesn't seem to be matched. It could mos def be me not knowing though \$\endgroup\$ – alexwc_ Aug 30 '16 at 20:06
  • 1
    \$\begingroup\$ @leftside It was a typo. Somehow I must have deleted part of that line. It is fixed now, along with a few other changes. \$\endgroup\$ – Mike Brant Aug 30 '16 at 20:20
  • \$\begingroup\$ @leftside Also just made a few changes to complete the code example. \$\endgroup\$ – Mike Brant Aug 30 '16 at 20:27
1
\$\begingroup\$

This looks actually pretty good.

  • Try to stick to lowerCamelCase for JavaScript, so largeToThumb instead of large_to_thumb.
  • One liner functions like large_to_thumb who are only called once, should probably get inlined
  • I like the variable name imageHovered, you should name the first parameter of thumb_to_large that way
  • In one place you use .product-image, in the other places (including HTML) you don't, that's a bit confusing
  • The more I think about it, the more you probably wanted to just pass $('.product-image .main-image') to thumb_to_large, because now you are re-getting that main image in the called function..
  • Avoid getting the same selection multiple times, query selectors or some of the slowest operators.

I tried to rewrite your code, and I am missing something, but you should get the gist. Feel free to point out where I went wrong.

function large_to_thumb(imageHovered, mainLarge, mainThumb){
    $(imageHovered).attr('src', mainThumb).data('large', mainLarge);
}

function thumb_to_large( hoveredImage ){
    var $mainImage = $('.product-image .main-image'),
        $hoveredImage = $(hoveredImage),
        mainLarge = $mainImage.attr('src'),
        mainThumb = $mainImage.data('gallery-image'),
        newMainLarge  = $hoveredImage.data('large'),
        newMainGalleryImage = $hoveredImage.data('src');

    // 1. put gallery image data-large into .main-large src
    $mainImage.attr('src', newMainLarge );
    // 2. put gallery image src into .main-large data-gallery-image
    $mainImage.data('gallery-image', newMainGalleryImage);
    // 3. update the hovered image to show what was in the main
    $hoveredImage.attr('src', mainThumb)
                 .data('large', mainLarge);
}


$('.product-gallery img').on('mouseenter touch', function(){
    thumb_to_large(this);
});
li {
  display:inline;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div class="product-image">
    <img class="main-image" src="http://placehold.it/300x300" data-gallery-image="http://placehold.it/150x150">
    <div class="product-gallery">
      <ul>
        <li>
          <img src="http://placehold.it/149x149" data-large="http://placehold.it/299x299">
        </li>
        <li>
          <img src="http://placehold.it/148x148" data-large="http://placehold.it/298x298">
        </li>
      </ul>
    </div>
  </div>

\$\endgroup\$
  • \$\begingroup\$ Ah yes, camel casing. I was in the midst of PHP coding and didn't revert to JS style - good suggestion. I'll look at this when home. \$\endgroup\$ – alexwc_ Aug 30 '16 at 17:08
  • \$\begingroup\$ the gallery images don't change after they been hovered the first time. \$\endgroup\$ – alexwc_ Aug 31 '16 at 2:24

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.