HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Making a ul list element animation on hover

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
animationelementlistmakinghover

Problem

I have an experimental script that makes an animation of a ul list element background-position, when the list element is hovered. Is there another way to manage this task or just optimize this code?

I want to make a menu that has elements with an animated background and have an idea about rotoscope style design or something in that direction. I'm thinking in cross-browser compatibility way too. In this stage this code runs fine in Opera, Internet Explorer 6, Chrome, Firefox 2.0, and Firefox 4.0.

jsFiddle

$(document).ready(function(){
  $("li.animate").hover(
    function () {

      var param = '0 0';
      ids = setInterval(function() {
        if ( c >= 5 ) {
          c = 1;
          $('.animate').css('backgroundPosition', '0 0');
        }
        else {
          param = (-100 * c++ )+'px 0';
          //alert(param);
          $('.animate').css('backgroundPosition', param);
          if ( c > 4 ) $('.animate').css('backgroundPosition', '0 0');
        }
      }, 40);
    },
    function () {
      $('.animate').css('backgroundPosition', '0 0');
      clearInterval(ids);
    }
  );
});

Solution

First of all i mention again, that i would use a GIF animation to solve this and not a JS. Its a very suboptimal solution to a simple problem.

Your code in the code block is missing two var declarations you have in you jsFiddle example.

Here are the things i would change:

In the most cases, when you have to type the same value twice you do something wrong. Use variables to store your stuff. Makes code more maintainable, easy to read, faster in some cases etc... And give your variables names that speak for them selves. Since you don't write any comments, its even more important.

Cache your jQuery objects in variables. By selecting them again and again, your code gets slow and is hard to mainain. Here an example:

$animate = $("li.animate");
$animate.hover(/* blabalbal */);


Put all your code in a function and pass variable stuff (things that could change) as parameters. Always ask your self what could change if you want to reuse the same code for a different CSS / HTML configuration. (Like different size of the element)

var yourFunction = function( $elem ){
  $elem.each(function(){
   //do something
  });
};

$(function(){ //document ready
  yourFunction( $("whatever element you want to handle") );
});


Then you can call your function in the document ready and you don't have to write all your stuff into your document ready function.

In general split your code in as small generic functions as possible. (Morden browser compile functions that are used often, can make a huge performance differences)

Inside your hover functions you reselect $('.animate'). What are you doing if you have more then one element that has this class? You can refer to the element you are hovering with this inside your hover function:

$(something).hover(function(){ $(this) //give you back a jQuery selection of the hovered element 
   },function(){});


The biggest problem i see in your code is that you have declared your variables very globally, what are you doing if you need to apply this code twice on the same page?

Hope this helps.

Code Snippets

$animate = $("li.animate");
$animate.hover(/* blabalbal */);
var yourFunction = function( $elem ){
  $elem.each(function(){
   //do something
  });
};

$(function(){ //document ready
  yourFunction( $("whatever element you want to handle") );
});
$(something).hover(function(){ $(this) //give you back a jQuery selection of the hovered element 
   },function(){});

Context

StackExchange Code Review Q#3148, answer score: 2

Revisions (0)

No revisions yet.