JavaScript Image Slideshow

Question

I've created a JavaScript image slideshow, using only JavaScript (no jQuery). I want to become more familiar with JavaScript, which is why I didn't use jQuery.

My four requirements were:

It must work if JavaScript is disabled.
It must be responsive.
It cannot use jQuery.
It must work in IE 8.

The code works - I'm just looking to see if I could make it more efficient.

jsFiddle

// Get array with photos
var photos = document.getElementsByTagName("figure"); // Returns object of photos
var numPhotos = photos.length;
var shownPhotoIndex;

// Hide all photos except first by giving class of "hide" (if JS is not enabled, all photos will show)
function hidePhotos() {
    for (var i = 0; i < numPhotos; i++) {
      if (photos[i] !== photos[0]) {
        photos[i].className += " hide";
      } // end if statement
    } // end for statement
  } // end hidePhotos

function positionButtons() {
    var getImage = photos[0].children[0]; // IE 8 does not support photos[0].firstElementChild;
    var imageHeight = (getImage.height);
    var imageWidth = (getImage.width);
    var paddingTop = (imageHeight / imageWidth) * 100;
    paddingTop = +paddingTop.toFixed(2);
    var percent = paddingTop + "%";
    document.getElementById("previous").style.paddingBottom = percent;
    document.getElementById("next").style.paddingBottom = percent;
  }
  
  // Add controls (if JS is not enabled; controls will not be present)
function addControls() {
    var slideshow = document.getElementById("slideshow");
    // Create buttons
    var spanNext = document.createElement("span");
    var spanPrevious = document.createElement("span");
    // Give buttons IDs
    spanNext.setAttribute("id", "next");
    spanPrevious.setAttribute("id", "previous");
    // Add buttons to slideshow div
    slideshow.appendChild(spanNext);
    slideshow.appendChild(spanPrevious);
    // Add content to buttons
    document.getElementById("next").innerHTML = "<p>>></p>";
    document.getElementById("previous").innerHTML = "<p><<</p>";
    // Calculate position of buttons
    positionButtons();
  } // end addControls
  // Find curently shown photo

function findShownPhoto() {
    for (var i = 0; i < numPhotos; i++) {
      // if the image does not contain a class of "hide"
      if (photos[i].className.indexOf("hide") == -1) {
        shownPhotoIndex = i;
      } // end if statement  
    } // end for statement
  } // end findShownPhoto

function progressSlides() {
    var next = document.getElementById("next");
    // When the next button is clicked, show next photo
    next.onclick = function() {
        var nextPhoto;
        findShownPhoto();
        // If current photo is last photo, go to first photo
        if (shownPhotoIndex === (numPhotos - 1)) {
          nextPhoto = photos[0];
        } else {
          nextPhoto = photos[shownPhotoIndex + 1];
        }
        // Hide current photo
        // Add the "hide" class to the list of existing classes
        photos[shownPhotoIndex].className += " hide";
        // Show next photo by removing "hide" class
        // Create a new string with classes that removes "hide"
        var newClass = nextPhoto.className.replace("hide", "");
        nextPhoto.className = newClass;
    }; // end next click
    var previous = document.getElementById("previous");
    previous.onclick = function() {
        var prevPhoto;
        findShownPhoto();
        // If current photo is first photo, go to last photo
        if (shownPhotoIndex === 0) {
          prevPhoto = photos[numPhotos - 1];
        } else {
          prevPhoto = photos[shownPhotoIndex - 1];
        }
        // Hide current photo
        photos[shownPhotoIndex].className += " hide";
        // Show previous photo
        var newClass = prevPhoto.className.replace("hide", "");
        prevPhoto.className = newClass;
    }; // end previous click
  } // end progressSlides
  
// Run JS after images have downloaded  
window.onload = function() {
  addControls();
  progressSlides();
};

hidePhotos();

/* INSTRUCTIONS
 
All photos should be wrapped in figure elements and should be the same size
Compress photos before uploading (save originals)

*/

body {
  box-sizing:border-box
}

figure {
  margin:0
}

#slideshow {
  display:inline-block;
  padding:0;
  position:relative;
/* To position slideshow buttons */
  max-width:920px
}

.photo {
  margin:0;
  position: relative;
  /* To position captions */
}

.hide {
  display:none
}

#slideshow img {
  width:100%;
  border-top-left-radius:10px;
  border-top-right-radius:10px
}

#slideshow figcaption {
  background-color:#000;
  color:#fff;
  padding:1em;
  border-bottom-right-radius:10px;
  border-bottom-left-radius:10px;
  font-family:Helvetica,Arial,Tahoma,sans-serif;
  font-size:15px;
  line-height:150%;
  margin-top:-5px
}

#previous,#next {
  height:0;
/* Padding-bottom is added via JS; calculates based on height of image */
  position:absolute;
  top:0
}

#previous p,#next p {
  margin:0;
  display:table;
  padding:1em 10px;
  color:#fff;
  font-family:Arial;
  font-size:24px;
  background-color:#000;
  text-align:center;
  font-weight:700;
  opacity:.8;
  position:absolute;
/* positioned in relation to the #previous and #next spans */
  top:50%;
  margin-top:-1.6em
/* previously used transformY(-50%) to bring arrow up half the width of itself; not supported in IE 8 */
}

#previous p {
  left:0;
  border-radius:0 5px 5px 0
}

#next p {
  right:0;
/*moves button to right corner of span instead of left */
  border-radius:5px 0 0 5px
}

#previous {
  left:0
}

#next {
  right:0
}

#previous:hover,#next:hover {
  opacity:.5;
  cursor:pointer;
  cursor:hand
}

   <div id="slideshow">
                <figure class="image">
                    <img src="https://c1.staticflickr.com/9/8584/16136057529_e7b64928d0_z.jpg"  />
                    <figcaption>This is an example of a really long caption. Here I go. Do I wrap to a second line? Wrap wrap wrap wrap. Wrap Wrap Wrap Wrap Wrap Wrap Wrap Wrap Wrap Wrap Wrap</figcaption>
                </figure>
                <figure class="image">
                    <img src="https://c4.staticflickr.com/8/7495/16322256485_08ee0ee36f_z.jpg" />
                    <figcaption>Insert caption</figcaption>
                </figure>
                <figure class="image">
                    <img src="https://c2.staticflickr.com/8/7474/16120961661_8dc12962dd_z.jpg" />
                    <figcaption>Insert caption</figcaption>
                </figure>

"1.) It must work if JavaScript is disabled." - how are you going to accomplish this? — somethinghere
– somethinghere, Commented Jun 29, 2015 at 14:16
All of the images load on the page, one below the other, if JS is disabled. JS is what hides the images. — amylynn83
– amylynn83, Commented Jun 29, 2015 at 14:33

Community · Accepted Answer · 2020-06-10 13:24:26Z

Comments

There are numerous problems with your comments.

      } // end if statement
    } // end for statement
  } // end hidePhotos

It is blatantly obvious what a } means; you don't need to tell us.

var photos = document.getElementsByTagName("figure"); // Returns object of photos

No, getElementsByTagName does not return an object of photos: it returns an array of elements (that may be photos).

// Hide all photos except first by giving class of "hide" (if JS is not enabled, all photos will show)

You wrote something like a few more times.

Yes. If JavaScript is not enabled, the photos will not show, as with the rest of your JavaScript. This is why you should use a <noscript> in your HTML.

Put comments after a line of code. Putting small, frequent comments between lines make reading your code much more difficult.

Indentation

You need to be consistent with your indentation. Sometimes you are using 4 spaces (or 1 tab), other times you are using 2 spaces.

I personally use 1 tab.

The code

Now for a review on your code.

var numPhotos = photos.length;

This is a waste; just as easy and just as convenient to use photos.length.

function hidePhotos() {
    for (var i = 0; i < numPhotos; i++) {
      if (photos[i] !== photos[0]) {
        photos[i].className += " hide";
      } // end if statement
    } // end for statement
  } // end hidePhotos

If you are trying to skip over the first element of the photos array, why don't you just start i at 1? That way, you won't have to constantly do conditional checking, which probably safes speed and efficiency.

You are setting up all these buttons and controls in your JavaScript code... wouldn't it be easier (and more efficient) to just put the buttons and controls in your HTML and then to bind functions from your JavaScript to them?

This may not be true; it was a little hard to tell with the snippet.

When someone first opens up your webpage, all the pictures are going to be seen for a second, and then they will disappear.

To rid of this, set the image class to be hidden by default on start-up, and then, instead of looping through and hiding them all, just make the first one visible.

photos[shownPhotoIndex].className += " hide";
// Show next photo by removing "hide" class
// Create a new string with classes that removes "hide"
var newClass = nextPhoto.className.replace("hide", "");

Don't do a few string operations and then change the class of an element; just set display style property to none.

Right now, to keep track of what picture you are on, you are setting the current element's id to "next".

Instead of interacting with the DOM to keep track of which element, try this:

Set a variable currentSlide to 0 (to start).

1.5. User clicks. Set current slide off.

If the user clicks the right arrow, currentSlide = (currentSide + 1) % photos.length.
If the user clicks the left arrow, you have to do some conditional checking to set it to photos.length - 1.

Then, you can set the slides like this:

photos[currentSlide].style.display = "";

And, with this variable, you already know what the current slide is, so you don't need findShownPhoto.

An entirely new idea for your system: since this is just an image slideshow, just create an object that holds an image url and a capt

Create a class for creating objects holding an image url and a caption.
Setup all the images as instances of this class and put them into an array.
When the user changes slide, just change the src property of an img tag and then innerHTML of a figcaption tag with the properties of the object.

Your HTML would be reduced to a single img and figcaption.

Here is a rough idea of what I am talking about:

function SlideshowImage(imgSrc, caption) {
    this.imgSrc = imgSrc;
    this.caption = caption;
}

var slides = [new SlideshowImage("foo", "bar"), new SlideshowImage("spam", "eggs")];

Then, when changing slide:

document.getElementById("image").src = slides[currentSlide].imgSrc;
document.getElementById("caption").innerHTML = slides[currentSlide].caption;

This answer took me a while to write and I was a little drowsy as I wrote it. If anything is too confusing, just let me know in a comment.

You are right: I am having a problem where all of the images show on start-up and then hide. I'm not sure what to do about this. You suggested: "Set the image class to be hidden by default on start-up, and then, instead of looping through and hiding them all, just make the first one visible." Do you mean that I should add .image {display: none} in my CSS? In which case, the images would be hidden even for people without JS. Or am I misunderstanding? — amylynn83
– amylynn83, Commented Jul 1, 2015 at 15:21
I don't include the controls in my HTML because I don't want them shown when JS is disabled. Otherwise, when ppl clicked on them, they wouldn't do anything. — amylynn83
– amylynn83, Commented Jul 1, 2015 at 15:23
@amylynn83 Precisely. Set all the .images to display:none. Then, at start, you only need to go the to the first .image and set that so it is visible. — SirPython
– SirPython, Commented Jul 1, 2015 at 15:23
@amylynn83 That is why you include a <noscript> in your page. If someone were to open up your page without JavaScript enabled, they will see whatever HTML is inside the <noscript>, which could be telling them to turn on JavaScript if they wish to use the webpage's features. — SirPython
– SirPython, Commented Jul 1, 2015 at 15:25

Stack Exchange Network

JavaScript Image Slideshow

1 Answer 1

Comments

Indentation

The code

You must log in to answer this question.

Hot Network Questions

JavaScript Image Slideshow

1 Answer 1

Comments

Indentation

The code

You must log in to answer this question.

Related

Hot Network Questions