5
\$\begingroup\$

I have a simple gallery that hides and shows images. It works fine, however, I am not satisfied with my approach, and my code seems redundant. Can you check my code and give a better idea on how I can improve it?

HTML

<div class="big_img_wrapper">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_1.JPG" id="big_img_1" class="big_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_2.JPG" id="big_img_2" class="big_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_3.JPG" id="big_img_3" class="big_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_4.JPG" id="big_img_4" class="big_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_5.JPG" id="big_img_5" class="big_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_6.JPG" id="big_img_6" class="big_img">
            </div>
            <div class="thumbs_img_wrapper">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_1.jpg" id="thumbs_img_1" calss="thumbs_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_2.jpg" id="thumbs_img_2" calss="thumbs_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_3.jpg" id="thumbs_img_3" calss="thumbs_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_4.jpg" id="thumbs_img_4" calss="thumbs_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_5.jpg" id="thumbs_img_5" calss="thumbs_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_6.jpg" id="thumbs_img_6" calss="thumbs_img">
            </div>

CSS

.big_img_wrapper, .big_img_wrapper img{
        width: 370px;
        height: 246px;
        /*display: none;*/
    }
    .thumbs_img_wrapper{
        padding:0;
    }
    .thumbs_img_wrapper img{
        width: 111px;
        height: 70px;
        margin: 14px 0 0 14px;
    }
    #thumbs_img_1, #thumbs_img_4{
        margin: 14px 0 0 0;
    }

jQuery

$(document).ready(function(){

    $('img.big_img').hide(); // Hides all big images
    $('img#big_img_1').fadeIn('slow'); // Serve as default image

    $('img#thumbs_img_1').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_1').fadeIn('slow'); //Slowly fades in selected image
    });

    $('img#thumbs_img_2').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_2').fadeIn('slow'); //Slowly fades in selected image
    });

    $('img#thumbs_img_3').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_3').fadeIn('slow'); //Slowly fades in selected image
    });
    $('img#thumbs_img_4').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_4').fadeIn('slow'); //Slowly fades in selected image
    });
    $('img#thumbs_img_5').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_5').fadeIn('slow'); //Slowly fades in selected image
    });
    $('img#thumbs_img_6').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_6').fadeIn('slow'); //Slowly fades in selected image
    });
});
\$\endgroup\$
  • \$\begingroup\$ This would be better if it was a snippet \$\endgroup\$ – Pimgd Nov 13 '14 at 12:25
2
\$\begingroup\$

Also you can keep path in data structure.

<div class="big_img_wrapper"></div>
<div class="thumbs_img_wrapper"></div>

<script>
  var i, big_image_urls, thumbs_image_urls, gallery_size;

  big_image_urls = [
    'http://cdn.sstatic.net/stackexchange/img/hiring/photos/big/1.jpg',
    'http://cdn.sstatic.net/stackexchange/img/hiring/photos/big/2.jpg',
    'http://cdn.sstatic.net/stackexchange/img/hiring/photos/big/3.jpg',
    'http://cdn.sstatic.net/stackexchange/img/hiring/photos/big/4.jpg',
    'http://cdn.sstatic.net/stackexchange/img/hiring/photos/big/5.jpg'
  ];

  thumbs_image_urls = [
    'http://cdn.sstatic.net/stackexchange/img/hiring/photos/thumbs/1.jpg',
    'http://cdn.sstatic.net/stackexchange/img/hiring/photos/thumbs/2.jpg',
    'http://cdn.sstatic.net/stackexchange/img/hiring/photos/thumbs/3.jpg',
    'http://cdn.sstatic.net/stackexchange/img/hiring/photos/thumbs/4.jpg',
    'http://cdn.sstatic.net/stackexchange/img/hiring/photos/thumbs/5.jpg'
  ];

  i = 0;
  gallery_size = thumbs_image_urls.length;

  while (i < gallery_size) {
    $('.thumbs_img_wrapper').append(
      '<img src="' + thumbs_image_urls[i] + '" id="' + i + '" class="thumbs_img">'
    );
    i++;
  }

  $('.big_img_wrapper').append(
    '<img src="' + big_image_urls[0] + '" id="big_img_' + 0 + '" class="big_img">'
  );

  $('.thumbs_img').click(function(){
    var id;
    id = $(this).attr('id');
    $('.big_img')
      .hide()
      .attr('src', big_image_urls[id])
      .fadeIn('slow');
  });
</script>
\$\endgroup\$
  • \$\begingroup\$ You have good approach, I appreciate it. I use the approach given earlier and it works fine. I also tried yours and it works fine also, however all the big images are displayed after page reload, and it will only display single image after clicking the thumbnails. \$\endgroup\$ – Rodge Nov 13 '14 at 13:16
  • \$\begingroup\$ @Rodge Ugh, sorry. I edited my answer and made an example: jsfiddle.net/ohbzm7ky/2 \$\endgroup\$ – Bob Napkin Nov 13 '14 at 13:53
  • \$\begingroup\$ thanks khusaenov. It also works fine. I'll try this to my other page. \$\endgroup\$ – Rodge Nov 14 '14 at 1:38
  • \$\begingroup\$ @Rodge welcome! \$\endgroup\$ – Bob Napkin Nov 14 '14 at 4:10
3
\$\begingroup\$

replace all .click() functions with:

$('img.thumbs_img').click(function(){
        $('img.big_img').hide(); // Hides all big images
        var id = $(this).attr('id');
        id = id.replace("thumbs_img_", "big_img_");
        $('img#'+id).fadeIn('slow'); //Slowly fades in selected image
    });
\$\endgroup\$
  • \$\begingroup\$ Hey Edan Feiles. This is better however when tried this one, it din't work. \$\endgroup\$ – Rodge Nov 13 '14 at 9:11
  • \$\begingroup\$ did you changed the html from calss= to class=? \$\endgroup\$ – Edan Feiles Nov 13 '14 at 9:13
  • \$\begingroup\$ Yah! Thanks again and sorry for my typo errors. The problem now comes when I clicked on the small image, the large one disappear and didn't show the other one. \$\endgroup\$ – Rodge Nov 13 '14 at 9:21
  • \$\begingroup\$ Cool! It worked. I replace id = str.replace to id = id.replace and works fine. Thanks guys for your great solutions. I really appreciate it \$\endgroup\$ – Rodge Nov 13 '14 at 9:31
2
\$\begingroup\$

What's the point of having thumbnails if you're just going to have the user download them plus the full sized images up front? The whole idea behind thumbnails is that you present the user with a smaller version so they can decide for themselves whether or not they want to take the time/spend the bandwidth to view the larger image.

With a few modifications to your markup, you can make this a lot easier for yourself:

<div class="thumbs_img_wrapper" id="gallery">
    <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_1.jpg" data-full-path="path/to/original_1.jpg" />
    <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_2.jpg" data-full-path="path/to/original_2.jpg" />
    <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_3.jpg" data-full-path="path/to/original_3.jpg" />
    <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_4.jpg" data-full-path="path/to/original_4.jpg" />
    <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_5.jpg" data-full-path="path/to/original_5.jpg" />
    <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_6.jpg" data-full-path="path/to/original_6.jpg" />
</div>

With a little help from event delegation, you can make this very efficient (disclaimer: this probably has some errors, but it will give you an idea of what to do):

document.getElementById('gallery').addEventListener("click", function(ev) {
    if (ev.target.tagName.toLowerCase() == 'img') {
        var fullPath = ev.target.getAttribute('data-full-path');
        // do stuff with fullPath that will load & display the full sized image
    }
});

Your images are missing alt attributes. If it is unrealistic to provide useful alt information due to dynamically content, just use an empty value (eg. alt="").

Your images are also missing dimensions. Adding dimensions will diminish/eliminate document reflow as the images load.

\$\endgroup\$

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.