Fading banner like text with jQuery

Question

I have a title text, which I want to fade into a box. Afterwards, a subtitle should fade in beneath it. Once both are visible, they should fade out, and the next set should fade in, in the same fashion.

This I have created, however, I have my doubts on how well it runs, that is, if it takes to much CPU for the browser. One of my concerns are also the recursion depth of Javascript.

I have the following code (which I have also included in a jsfiddle, along with CSS and HTML http://jsfiddle.net/ukewY/1/)

var no = 3;

function fadeText(id) {
  // Fade in the text
  $("#text" + id).animate({
    opacity: 1,
    left: '+=50'
  }, 5000, function() {
    // Upion completion, fade in subtext
    $("#subtext" + id).animate({
      opacity: 1,
    }, 5000, function() {
      // Upon completion, fade the text out and move it back
      $("#subtext" + id).animate({
        opacity: 0, 
      }, 1000, function() {
        $("#text" + id).animate({
          opacity: 0,
          left: '+=200'
        }, 3000, function() {
          // Yet again upon completion, move the text back 
          $("#text" + id).css("left", "19%");
          $("#subtext" + id).css("left", "10%")
          fadeText((id % no) + 1);
        });
      });
    });
  });
}

$(document).ready(function() {
  fadeText(1);
});

My question is if this is the right way to do it; or if there is a better, maybe none-recursive, way of doing this?

PS. As this is for a banner for a website, I do not worry about id being to big, as the people have probably moved on since.

Answer 1

The recursion seems fine to me, but there are a few other things I spotted:

• You probably want to dynamically read the number of titles, rather than having to specify them at the top of the script.
• You are using the same selectors $("#text" + id) and $("#subtext" + id) twice in each title. You should only do it once, and assign it to a variable. This means you only have one function call, not two.
• You may want to use the eq() selector to eliminate the need for $("text" + id) and make it tidier
• There is a couple of data maps that you are passing to animate() that end with commas even though there is only 1 value (specifically "{opacity: 0,}"). This will cause problems on some browsers.
• I'm not 100% sure, but i think calling a function from within itself is bad, you should use setTimeout (and use an anonymous function if you need to pass the function some parameters to avoid it using eval())
• I know you said it didn't matter, but id getting too big can happen if a user just leaves your page open (e.g. answers the phone then has to rush out). They shouldn't come back to an error.
• You can use $(DO STUFF) rather than $(document).ready(DO STUFF)

I took car of these and took the liberty of tweaking your code to the following:

function fadeText() {
    thistext = $text.eq(titleid);
    thissubtext = $subtext.eq(titleid);
    thistext.animate({
        opacity: 1,
        left: '+=50'
    }, 5000, function () {
        thissubtext.animate({
            opacity: 1
        }, 5000, function () {
            thissubtext.animate({
                opacity: 0
            }, 1000, function () {
                thistext.animate({
                    opacity: 0,
                    left: '+=200'
                }, 3000, function () {
                    thistext.css("left", "19%");
                    thissubtext.css("left", "20%");
                    if (titleid != $text.length - 1) {
                        titleid++;
                    } else {
                        titleid = 0;
                    }
                    setTimeout(fadeText, 0);
                });
            });
        });
    });
}

var titleid=0;

$(function () {
  $text = $("span.floating-text");
  $subtext = $("span.floating-subtext");
  fadeText();
});