Cleaning up a rotating image gallery

Question

I have created a nice little image gallery for the web. I set out wanting 3 things. 1. The whole gallery had a basic level responsiveness to it. 2. The gallery would take images of any size, and display them without cropping. 3. The gallery would run automatically. If you look here(JSFiddle) I have done just that. The automatic slide show is just a small bit of jQuery.

setInterval(function () {
  $(".image_background").last().animate({
        left: '-=1000'
        }, 1000, 'swing', function() {
          var top_element= $(this).detach();
          top_element.css({"left": 0});
          $(".image_gallery").prepend(top_element);
    })}, 2000);

I am quite happy with it. It's the css bit that I am not so sure about.

.main_image_gallery{
  display: inline-block;
  position: absolute;
  height: 50%;
  width: 30%;
  z-index: 2;
  top: 30%;
  left: 3%;
  background-color: #4e9ba1;
  box-sizing: border-box;
  overflow: hidden;
}
.image_background{
  background-color: #4e9ba1;
  display: inline-block;
  position: absolute;
  height: 100%;
  width: 100%;
}
.gallery_image{
  margin: auto;
  position: absolute;
  top: 0; left: 0; bottom: 0; right: 0;
  max-width: 100%;
  max-height: 100%;
}

For reference, here's the HTML

<div class="main_image_gallery image_gallery">
    <div class="image_background">
      <img class="gallery_image" src="http://www.vectortemplates.com/raster/batman-logo-big.gif" ></img>
    </div>
    <div class="image_background">
      <img class="gallery_image" src="http://www.vectortemplates.com/raster/batman-logo.gif" ></img>
    </div>
    <div class="image_background">
      <img class="gallery_image" src="http://th09.deviantart.net/fs70/PRE/f/2011/290/8/e/batman_logo_circleless_by_machsabre-d4d6s56.png" ></img>
    </div>
  </div>

I feel like there is a simpler way to do it. Any thoughts on how I can clean it up, maybe improve the responsive aspects?

Answer 1

For the css part, I started from the updated fiddle @cimmanon provided. I basically agree with all the things he added. I also went ahead and turned your markup into a ul as he (or she) correctly suggested. I would like to provide my own insights as well:

• You are closing your img tags, however they can never contain any content so the closing tags are redundant (and perhaps even invalid). You can either make them self-closing (<img ... />) or just omit them in html5.

• You are using a lot of different classes in your markup. There is no real need for that; all it does is make your code longer and harder to read. I would go for selectors like .image-gallery img instead of .gallery_image. I admit that in theory this is slightly less performant, but I doubt you'll be able to see, or even measure, the difference. And it makes your markup a lot easier to maintain (less code when adding slides, and less chance of making typos in all those class names).

• I would definitely not animate the left property, but go for a transform: translate3d in stead. It is a lot better for performance as explained here.

• I would use css transitions, instead of the jQuery animate. It is again better for performance, and it I believe it makes your code a lot better structured. If you decide to change anything in your animation, you just have to update your css and you don't have to start digging into your javascript. I'm a big fan of only using Javascript for thing that can't be done through css.

I went ahead and applied my suggestions to the fiddle, which you can find here: http://jsfiddle.net/eL0vaL44/3/

Answer 2

Your markup looks pretty good. You're using just enough elements to get the desired effect and no more. Semantically speaking, a list might be more appropriate here and would give you the same structure.

It's unclear why your .main_image_gallery element is absolutely positioned. If it's because you want to keep the child elements contained within, you should be using position: relative instead. If it's so you can have the container sized relative to the parent element (in this case, the body element), there's a different technique you can use:

https://stackoverflow.com/questions/12121090/responsively-change-div-size-keeping-aspect-ratio

.main_image_gallery{
  width: 50%;
  background-color: #4e9ba1;
  box-sizing: border-box;
  overflow: hidden;
  position: relative;
}
.main_image_gallery:before {
   content: '';
   display: block;
   padding-bottom: 150%;
}
.image_background{
  background-color: #4e9ba1;
  position: absolute;
  top: 0;
  height: 100%;
  width: 100%;
}

http://jsfiddle.net/eL0vaL44/2/

You don't need to specify display: inline-block if you're absolute positioning an element. Absolutely positioning an element will cause it to shrink to its contents.