3
\$\begingroup\$

As the question says....Can this be done any better? I could introduce more variables to cut down on function number, but does it make sense? So this is a simple cross-fade between two images :

var slide_number = 1;
var slide_number2 = 2;
var slide1 = new Image()
slide1.src = "img/top_slideshow/slideshow-1.jpg"
var slide2 = new Image()
slide2.src = "img/top_slideshow/slideshow-2.jpg"
var slide3 = new Image()
slide3.src = "img/top_slideshow/slideshow-3.jpg"
var slide4 = new Image()
slide4.src = "img/top_slideshow/slideshow-4.jpg"
var slide5 = new Image()
slide5.src = "img/top_slideshow/slideshow-5.jpg"
var slide6 = new Image()
slide6.src = "img/top_slideshow/slideshow-6.jpg"
var slide7 = new Image()
slide7.src = "img/top_slideshow/slideshow-7.jpg"
var slide8 = new Image()
slide8.src = "img/top_slideshow/slideshow-8.jpg"


function set_first_image() {
    document.images.slide.src = eval("slide"+slide_number+".src");
}
function set_second_image() {
    document.images.slide2.src = eval("slide"+slide_number2+".src");
}
function first_image_fade_in(){
    document.getElementsByClassName('slideshow')[0].setAttribute('style', 'opacity: 0;');
    slide_number += 2;
    if (slide_number >= 9){
        slide_number = 1;
    }
    setTimeout(function() {set_first_image()}, 2000);
}
function first_image_fade_out(){
    document.getElementsByClassName('slideshow')[0].setAttribute('style', 'opacity: 1;');
}
function second_image_fade_in(){
    document.getElementsByClassName('slideshow2')[0].setAttribute('style', 'opacity: 0;');
    slide_number2 += 2;
    if (slide_number2 >= 9){
        slide_number2 = 2;
    }
    setTimeout(function() {set_second_image()}, 2000);
}
function second_image_fade_out(){
    document.getElementsByClassName('slideshow2')[0].setAttribute('style', 'opacity: 1;');
}

function slideshow(){
    setTimeout(function() {first_image_fade_in()}, 20000);
    setTimeout(function() {second_image_fade_out()}, 20000);
    setTimeout(function() {first_image_fade_out()}, 40000);
    setTimeout(function() {second_image_fade_in()}, 40000);
    setTimeout(function() {slideshow()}, 40000);
}
\$\endgroup\$
1
\$\begingroup\$

I think you're doing it a tiny bit too complicated. There are so many variables and timeouts that it's really complicated and hard to read or understand.

Also I'm not sure about your list of images. Why do you create images, when you only access their src attribute? A simple array would suffice here, utilizing the browser's cache (no need to re-download anyway).

Also keep in mind that you don't really have to fade both images. It's enough to only fade the topmost image.

You can try my simplified approach in this jsFiddle.

The basic HTML is minimalistic:

<div class="imgbox">
  <img id="img1" onload="fadeImage();">
  <img id="img2" onload="fadeImage();">
</div>

You only need some CSS to align the images one atop the other, plus setup the transparency/transition:

/* Make sure the second image isn't visible at first */
#img2 {
  opacity: 0;
  transition: opacity .5s;
}

Last but not least the actual JavaScript:

var imgidx = 0;
var imgtoggle = false;

// Images for our slideshow - have to be at least two
var images = [
  'https://i.imgur.com/woXDBE1.png',
  'https://i.imgur.com/XntCAQK.png',
  'https://i.imgur.com/hIHVbAj.png'
];

// This function will initialize loading the next image in background
window.nextImage = function() {
  imgtoggle = !imgtoggle;

  imgidx = (imgidx + 1) % images.length;

  if (imgtoggle)
    document.getElementById('img2').src = images[imgidx];
  else
    document.getElementById('img1').src = images[imgidx];;
}

// This function will trigger the crossfade and then start the timer for the next switch
window.fadeImage = function() {
  if (imgtoggle)
    document.getElementById('img2').style.opacity = '1.0';
  else
    document.getElementById('img2').style.opacity = '0.0';
  window.setTimeout(nextImage, 2000);
}

// Now load our very first image to start the slideshow
document.getElementById('img1').src = images[0];

Whenever an image finishes loading - which can be instantly, once the file is cached - the transition is triggered and the timer for the next swap starts.

While imgidxidentifies the current image in the array, imgtoggle is used as a switch to simply alternate between both images, saving you from duplicating code.

If you'd like to fade to white (or any other color) for transitions, the code above could be modified, simply adjusting the opacity of an additional <div/> covering both images.

\$\endgroup\$
  • \$\begingroup\$ Simplification of the code is exactly what I was after. Love your solution, it is neat and it looks more professional. Thank you very much for the advice as well. I kept the variables from the previous versions of a strictly timed slideshow (i didn't like the messy timing errors due to image load times). But now with a cross-fade style transition and your solution I can easily rely on caching or pure load times. \$\endgroup\$ – Damian Chrzanowski Jul 23 '16 at 23:05
  • \$\begingroup\$ i had to put the first load document.getElementById('img1').src = images[0]; of the picture as an function that loads after the whole body is loaded, otherwise it threw an error that the id img1 doesn't exist...understandably. Thanks for your help again Mario. \$\endgroup\$ – Damian Chrzanowski Jul 24 '16 at 10:44
  • \$\begingroup\$ @DamianChrzanowski Yes, of course. In jsFiddle the whole JavaScript is essentially "onload" code by default. You can only have that line outside, if that line follows after the actual images in your HTML. \$\endgroup\$ – Mario Jul 24 '16 at 19:31
  • \$\begingroup\$ Still new to the whole thing. Thanks again for the tips :) \$\endgroup\$ – Damian Chrzanowski Jul 24 '16 at 22:20
1
\$\begingroup\$

I must say it was interesting running your code with some sample images-

  1. You have so many variables declared especially the slides - You should try using a Create Factory method for your Images. Example from Creating an array of Image Objects

// Image factory
var createImage = function(src, title) {
  var img   = new Image();
  img.src   = src;
  img.alt   = title;
  img.title = title;
  return img; 
};

// array of images
var images = [];

// push two images to the array
images.push(createImage("http://www.lostarkgames.com/images/pokemonlogo.jpg", "foo title"));
images.push(createImage("http://4.bp.blogspot.com/-Jv6VFqBbTIM/VNUbs8ZehLI/AAAAAAAATD8/Dwipe_uNxTQ/s1600/Pok%C3%A9mon_Gotta_Catch_'Em_All_1.png", "bar title"));

// output
console.log(images);

  • When you use fade out, it's best to fade out slowly before making the image transparent e.g 1-> 0.2->0 . It's just for the user experience. the animation lacks consistency based on the timing, some images stayed longer than 20 seconds
\$\endgroup\$
  • \$\begingroup\$ Thanks for the quick reply and sorry for a late reply on my behalf. I'm only starting JS, so I really just wanted assurance on how does this code presents itself and it seems to be ok :) . Thank you very much for the tip regarding the array-image-packing (I especially like putting in the titles, alt, etc. at the same time). \$\endgroup\$ – Damian Chrzanowski Jul 23 '16 at 12:15

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.