Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this function in this fiddle

JavaScript:

var newsTicker = function (ele) {
    var eles = ele.find('ul li'),
        indexEle = 0,
        dataEle = ele.find('.ticker-post'),
        rotateChars = function (title) {
            dataEle.text('');
            indexCut = 0;
            rotateCharsTimer = setInterval(function () {
                if (dataEle.text().length == title.length) {
                    clearInterval(rotateCharsTimer);
                } else if (dataEle.text().length < title.length) {
                    text = dataEle.text().concat(title.substr(indexCut, 1));
                    dataEle.text(text);
                };
                if (title.length - 1 > indexCut) indexCut++;
                else indexCut = 0;
            }, 90);
        },
        loopLs = function () {
            loopEle = eles.get(indexEle);
            eleHref = $(loopEle).data('href');
            eleTitle = $(loopEle).data('title');
            dataEle.attr('href', eleHref);
            if (typeof rotateCharsTimer != 'undefined') {
                dataEle.fadeOut();
                clearInterval(rotateCharsTimer);
            }
            dataEle.fadeIn();
            rotateChars(eleTitle);
            if (eles.length - 1 > indexEle) indexEle++;
            else indexEle = 0;
        }
    loopLs();
    setInterval(function (){loopLs()},9400);
}

HTML:

<div id="news-ticker">
    <div class="ticker-title">Breaking News:</div>
    <ul>
        <li data-href="#1" data-title="No 10 armed police arrested over hardcore pornography"></li>
        <li data-href="#2" data-title="EDF extends life of UK nuclear plants"></li>
        <li data-href="#3" data-title="Superwoman Stephanie Flanders on how to do it all"></li>
        <li data-href="#4" data-title="Will Liverpool deliver for Suárez? Can Arsenal last?"></li>
    </ul>
    <a class="ticker-post"></a>
</div>

Is this the best way to do this?

share|improve this question
add comment

2 Answers 2

up vote 5 down vote accepted

The HTML markup should be semantic, such that it degrades gracefully in the absence of JavaScript:

<ul>
  <li><a href="#1">No 10 armed police arrested over hardcore pornography</a></li>
  <li><a href="#2">EDF extends life of UK nuclear plants</a></li>
  <li><a href="#3">Superwoman Stephanie Flanders on how to do it all</a></li>
  <li><a href="#4">Will Liverpool deliver for Suárez? Can Arsenal last?</a></li>
</ul>

To make that work, the CSS should not contain a ul { display: none } rule. Instead, the JavaScript code should be responsible for suppressing the normal display.


You have a bug in the timing that causes each ticker item to briefly fade out and fade in again, causing an undesirable flashing effect. The problem is that you don't wait for the fade-out to complete before proceeding. The fix is to use the complete callback of .fadeOut(). In order to fade out the correct element, it's easier to hang on to a $current jQuery object, advancing it only after the fade-out has completed.


You forgot to localize the variables in your inner functions using var. In rotateCharsTimer, the substring operation could be simplified. For readability, I prefer to write the newsTicker(), rotateChars(), and loopLs() functions "normally" rather than as anonymous functions assigned to a variable. Also for readability, I suggest passing parameters to the helper functions explicitly.

function newsTicker($ele) {
  var $eles = $ele.find('ul > li'),
      $current = $eles.hide().first(),
      rotateCharsTimer;

  function rotateChars($ele) {
    var indexCut = 0;
    var title = $ele.data('title');
    rotateCharsTimer = setInterval(function () {
      if ($ele.text().length >= title.length) {
        clearInterval(rotateCharsTimer);
      } else {
        $ele.text(title.substr(0, indexCut++));
      }
    }, 90);
  }

  function loopLs($eles) {
    if (typeof rotateCharsTimer != 'undefined'){
      clearInterval(rotateCharsTimer);
    }
    $current.find('a').fadeOut('slow', function() {
      $eles.hide();  
      $current = $current.next().length ? $current.next()
                                        : $eles.first();
      rotateChars($current.show()
                          .find('a')
                          .text('')
                          .fadeIn());
    });
  }

  // Transfer the text into a data-title attribute
  $eles.find('a').each(function() {
    $(this).data('title', $(this).text());
  });
  loopLs($eles);
  setInterval(function(){loopLs($eles);}, 9400);
}

$ = jQuery;
newsTicker($('#news-ticker'));

jsFiddle with the recommendations

share|improve this answer
add comment

Your jQuery function was okay, but it's common practice now to put your animations into CSS wherever possible.

I've rewritten your code, moving as much as possible into CSS. I'll explain how and why below, or you can skip to the final solution here.

Semantically your links shouldn't be data-attributes as they control page navigation and need to be visible and identifiable as such.

<div id="news-ticker">
    <div class="ticker-title">Breaking News: </div>
    <ul>
        <li>
            <a href="http://www.thetimes.co.uk/tto/news/uk/crime/article3999682.ece">No 10 armed police arrested over hardcore pornography</a>
        </li>
        <li>
            <a href="http://www.thetimes.co.uk/tto/environment/article3999530.ece">EDF extends life of UK nuclear plants</a>
        </li>
    </ul>
</div>

You shouldn't use float: left for this, these components are inline, not floating.

#news-ticker .ticker-title {
    display: inline-block;
    margin-right: 12px;
}

#news-ticker ul { 
    display: inline-block;
    position: relative;
}

Next we want to make each news item invisible when not active, and we want them to all appear in the same place. A simple way to do this is to make their position absolute.

#news-ticker li {
    position: absolute;
    left: 0;
    width: 0;
    overflow: hidden;
    height: 1em;
    word-wrap: break-word;  
    opacity: 0
}

There are a few more things I've added here as well...

  1. width: 0: By making the width of the elements 0 they are invisible, and we can extend the width to show them a letter at a time.
  2. word-wrap: break-word: This helps us get the tick each letter at a time effect.
  3. overflow: hidden: stops us from seeing the rest of the sentence when the container isn't wide enough.

Next we make the animation. There's a good guide here about how they work.

#news-ticker li.tick {
    -webkit-animation: tick 5s linear;

}

@-webkit-keyframes tick {
  0% {
    width: 0;
  }
  5% {
      opacity: 1;
  } 
  90% {
    width: 550px;
    opacity: 1;
  }
  100% {
     opacity: 0
  }
}

This means that when we add the class .tick to one of our list elements it will animate from 0 width to 550px.

Now for our small amount of JavaScript to switch the classes and make the wrap work on letters instead of words.

$(function () {
   var $ticker = $('#news-ticker'),
   $first = $('li:first-child', $ticker);

Unfortunately there's no letter-wrap CSS attribute, so we have to cheat a little. Here I insert a blank space inbetween each letter, essentially making it a word.

   $('a', $ticker).each(function () {
       var $this = $(this),
         text = $this.text();
       $this.html(text.split('').join('&#8203;'));
   });

And then start the ticker. Every time an animation ends, it grabs the next (or first) element and continues:

   function tick($el) {
     $el.addClass('tick')
       .one('webkitAnimationEnd oanimationend msAnimationEnd animationend', function () {

         $el.removeClass('tick');
           var $next = $el.next('li');
           $next = $next.length > 0 ? $next : $first;
         tick($next);
     });
   }

   tick($first);

});
share|improve this answer
1  
Thanks a lot i got new ideas from your code style :) –  Saad Shahd Feb 8 at 13:14
    
@SaadShahd Also it's much more performant to keep animations in your CSS. –  Jivings Feb 8 at 18:48
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.