4
\$\begingroup\$

There's this website I'm working on that needs to swap images every few seconds, like a slideshow or image fade swap in a way, except that you need to target 5 IMG tag elements. I was able to make a weak version of that function. But i'm sure this could be done even better, considering that I still have a few bugs that needs to be fixed. If anyone could land me a hand and steer me in the right direction with this, that would be mucho appreciated.

Bugs:

  • Broswer tab switch for a minute or two, and come back and the images flicker a bit, until the recursive function readjust itself and keep on going.
  • Last IMG element is never swapped. I figured, by commenting a bit of code, what caused the problem. It's the conditional statement that determines if the function has ended and must recall itself. Altho I'm not still sure what needs to be done to fix that.

The function:

  • Shuffle a string array containing image path
  • jQuery .each() loop through all my IMG elements
  • Opacity Fade out current element with TweenMax
  • On complete, swap element 'src' path for a new one within the string array
  • Opacity Fade in current element with TweenMax
  • Increment index var
  • Check to see if we reached the end of array. If true, we re-shuffle our string array and recall the function (recursion). If false, keep going with the loop, repeat all steps for next element

FYI: I based my recursive function on this SO post

The Shuffle function is basically Fisher-Yates

Here is a JSFiddle demonstrating the current state of the function

The real HTML is more complete than that. Light version for example:

<img src="http://hudspecialisten.se/wp/wp-content/uploads/2013/02/fr%C3%A5geteckaen.png" class="img-contnr-1 img-contnr" style="max-width:100%;" alt="homepage pic 1" />

<img src="http://hudspecialisten.se/wp/wp-content/uploads/2013/02/fr%C3%A5geteckaen.png" class="img-contnr-2 img-contnr" style="max-width:100%;" alt="homepage pic 2" />

<img src="http://hudspecialisten.se/wp/wp-content/uploads/2013/02/fr%C3%A5geteckaen.png" class="img-contnr-3 img-contnr" style="max-width:100%;" alt="homepage pic 3" />

<img src="http://hudspecialisten.se/wp/wp-content/uploads/2013/02/fr%C3%A5geteckaen.png" class="img-contnr-4 img-contnr" style="max-width:100%;" alt="homepage pic 4" />

<img src="http://hudspecialisten.se/wp/wp-content/uploads/2013/02/fr%C3%A5geteckaen.png" class="img-contnr-5 img-contnr" style="max-width:100%;" alt="homepage pic 5" />

The recursive jQuery/TweenMax function:

var globvars = {
    imgArr : '',
    arrCount: 0,
    arrIndex: 0,
    totalImageContainer: 5,
    currentImageContainer: 1,
    currentIMGPath:''
}

$(function () {
    globvars.imgArr = ["http://lab1663.net/images/pam_holy_shitsnacks.png", "http://i.ytimg.com/vi/Ftb09o6O7sw/0.jpg", "http://doyoulikelikeme.files.wordpress.com/2012/03/cheryl-archer.png", "http://static.guim.co.uk/sys-images/Guardian/Pix/pictures/2013/7/10/1373464392141/archer-008.jpg", "http://aravan.files.wordpress.com/2012/04/malorygun.jpg", "http://dramachan.net/rp/src/1401166946857.jpg"];

    globvars.imgArr = shuffleArray(globvars.imgArr);
    globvars.arrCount = globvars.imgArr.length;

    setTimeout(function () {
        FadeChange(globvars.imgArr);
    }, 8000);
});

function shuffleArray(array) {
    var copy = [], n = array.length, i;

    // While there remain elements to shuffle…
    while (n) {

        // Pick a remaining element…
        i = Math.floor(Math.random() * n--);

        // And move it to the new array.
        copy.push(array.splice(i, 1)[0]);
    }

    return copy;
}

function FadeChange(myArray) {

    var lights = $('.img-contnr-1, .img-contnr-4, .img-contnr-2, .img-contnr-5, .img-contnr-3');

    var index = 0;
    $.each(lights, function (i) {
        var el = $(this);
        setTimeout(function () {
            TweenMax.to(el, 0.35, {
                opacity: 0, delay: 1, onComplete: function () {
                    el.attr('src', myArray[index]);
                    TweenMax.to(el, 0.35, { opacity: 1, delay: 1 });
                }
            });
            index++;

            if (index >= (myArray.length - 1)) {
                    globvars.arrCount = globvars.imgArr;
                    globvars.imgArr = shuffleArray(myArray);
                    setTimeout(function () {
                        FadeChange(globvars.imgArr);
                    }, 8000);
            }
        }, i * 8000);
    });
}
\$\endgroup\$
4
  • \$\begingroup\$ An idea: instead of staggering the delay, add the index as a parameter. When one finishes, set a timeout for the next index. \$\endgroup\$
    – Schism
    Commented Jul 24, 2014 at 20:30
  • \$\begingroup\$ Do you mean, that instead of the .each loop, I add the index as a parameter, FadeChange(myArray, index) and delay/settimeout the next call? I just want to make sure I understand \$\endgroup\$
    – IndieRok
    Commented Jul 24, 2014 at 20:40
  • \$\begingroup\$ Yup. Intuitively that should also solve your flickering issue. (Use @<name> to explicitly ping somebody; I believe I got a notification only because I was the only other one to comment.) \$\endgroup\$
    – Schism
    Commented Jul 24, 2014 at 21:05
  • \$\begingroup\$ @Schism: Ok! Noted. I shall try that idea and see what I can do! \$\endgroup\$
    – IndieRok
    Commented Jul 24, 2014 at 21:13

1 Answer 1

4
\$\begingroup\$
  1. Functions should be camelCase unless they're constructors. So fadeChange (also, fadeChange is a little vague - what's getting faded? What's being changed?)

  2. You probably don't need global variables of any kind. Closures should be fine.

  3. You have your quite a few magic numbers in there. A couple aren't bad for animation stuff, but in this case, they're repeated in multiple places (like the 8000ms interval). This can be a maintenance issue.

  4. I don't think you need more libraries; jQuery's .fadeOut()/.fadeIn() should be sufficient.

I'd suggest something like this, where you don't loop through all the images in one go, but just do one at a time, wait, then do another, then wait... etc.

$(function () {

    var FADE_INTERVAL = 8000,
        FADE_DURATION = 350;

    var images = $(".img-contnr"),
        imageIndex = 0,
        urlIndex = 0,
        imageUrls = shuffleArray([
          // a bunch of image urls here
        ]);

    function swapImage() {
        // fade out the current image
        images.eq(imageIndex).fadeOut(FADE_DURATION, function () {
            // set next image index
            imageIndex = (imageIndex + 1) % images.length;

            // swap the img src
            this.src = imageUrls[urlIndex];

            // get next url index
            urlIndex = (urlIndex + 1) % imageUrls.length;

            // if we've gone through all the urls, re-shuffle
            if (urlIndex === 0) {
                imageUrls = shuffleArray(imageUrls);
            }

            // fade back in, and set up the next swap
            $(this).fadeIn(FADE_DURATION, delayedSwap);
        });
    }

    function delayedSwap() {
      setTimeout(swapImage, FADE_INTERVAL);
    };

    // call delayedSwap to trigger the first swap
    delayedSwap(); 
 });

jsfiddle

\$\endgroup\$
7
  • \$\begingroup\$ That's it! This is really great. Thanks you so much. I guess the .each() loop was executing way to fast and with the setTimeout delaying everything, it just got really messed up and glitchy. I didn't even thought about exploiting JQuery's own fadeIn/fadeOut methods, good thinking. Duh, they both have an onComplete parameters too. Well, learned something new yet again! A nice clean recursive function. \$\endgroup\$
    – IndieRok
    Commented Jul 24, 2014 at 22:33
  • \$\begingroup\$ @IndieRok No prob, glad it was useful. \$\endgroup\$
    – Flambino
    Commented Jul 24, 2014 at 22:35
  • \$\begingroup\$ Can you explain to me this line. 'imageIndex = (imageIndex + 1) % images.length;' Why are doing it like this, instead of 'imageIndex++'? I'm just curious \$\endgroup\$
    – IndieRok
    Commented Jul 25, 2014 at 14:06
  • \$\begingroup\$ @IndieRok Sure, you could do images.eq(imageIndex++)... and skip the + 1 later on (and a similar thing for urlIndex). However, I'd rather do the index-update-calculation in one place. And preferably on one, easily-readable line. So it's just for clarity, and to have it all in one place. The ++ is a neat syntactical shortcut, but its not required. Sometimes I just think writing + 1 is clearer \$\endgroup\$
    – Flambino
    Commented Jul 25, 2014 at 15:06
  • \$\begingroup\$ My bad, I didn't explain myself very well. What about the modulo (?) part '% images.length'. Why are you adding this? thanks \$\endgroup\$
    – IndieRok
    Commented Jul 25, 2014 at 15:12

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.