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

jQuery-animated navigation menu

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

Problem

On the attached fiddle is what I hope will the be future UI for navigation on my own personal portfolio site. I was hoping you guys could critique the JavaScript I wrote to accomplish the most compatibility with the most browsers. I've tested as far back as IE 7 with this code and it works. Whereas the CSS solution is only IE 10+, and the jQuery Animate method is only usable in Chrome.

Below is the attached fiddle and all of the script I use to facilitate this process. Please be as brutal as possible. I am very new to JS and would like to know as much as possible!

http://jsfiddle.net/PhilFromHeck/um8Pp/

var key = "off";
var menuKey=["off","off","off","off"];
var menuID = [];
var menuState=[0,0,0,0];

$(document).ready(function () {
    init();
    $('.menu').mouseenter(function () {
      for (var i = 0; i  0){
          check = "yes";
          menuState[i] = menuState[i]-2;
          menuID[i].style.backgroundPosition="0px " + menuState[i] + "px";     
        }
      }
    }
  if(check=="yes"){
    setTimeout(doMove,1); // call doMove in 20msec
  }
  else{
    key = "off";
  }
  }
}

Solution

For beginner code, that's reasonably good.

  • Instead of calling setTimeout() yourself, why not let jQuery handle the animation for you with .animate()?



-
Your indentation is wrong, as evidenced by

}
  }
}


at the end. I believe that two spaces is too stingy for readability, and also encourages excessive nesting, which is poor programming practice. For example, the entire doMove() function should do nothing unless key == "on", so you could eliminate one layer of nesting by returning early:

function doMove() {
    if (key != "on") {
        return;
    }
    ...
}


  • You use magic numbers 4 and 100. Don't hard-code 4; detect it as menuID.length instead. You should be able to detect the height as well; it not, then hard-code 100 as a named constant rather than being casually embedded as a loop limit.



-
Storage of state feels clumsy. Use booleans (true and false) instead of strings ("on" and "off"). Naming could be better — I suggest menuElements rather than menuID, and animationPosition rather than menuState. You should be able to use fewer variables. For example, key and menuKey could be collapsed down to one scalar called activeMenuItem.

var activeMenuItem;
var menuElements = [];
var animationPosition = [0, 0, 0, 0];

$(document).ready(function () {
    init();
    $('.menu').mouseenter(function () {
        activeMenuItem = this;
        doMove();
    });
    $('.menu').mouseleave(function () {
        activeMenuItem = null;
    });
});

function doMove() {
    var check = false;
    for (var i = 0; i  0) {
            animationPosition[i] -= 2;
            menuElements[i].style.backgroundPosition="0px " + animationPosition[i] + "px";
            check = true;
        }
    }
    if (check) {
        setTimeout(doMove, 1); // call doMove in 20msec
    }
}


  • Initialization of menuID scales poorly with additional menu items. Try selecting all $('.menu') instead of individually named elements.

Code Snippets

function doMove() {
    if (key != "on") {
        return;
    }
    ...
}
var activeMenuItem;
var menuElements = [];
var animationPosition = [0, 0, 0, 0];

$(document).ready(function () {
    init();
    $('.menu').mouseenter(function () {
        activeMenuItem = this;
        doMove();
    });
    $('.menu').mouseleave(function () {
        activeMenuItem = null;
    });
});

function doMove() {
    var check = false;
    for (var i = 0; i < menuElements.length; i++) {
        if (menuElements[i] == activeMenuItem) {
            if (animationPosition[i] < 100) {
                animationPosition[i] += 2;
                menuElements[i].style.backgroundPosition="0px " + animationPosition[i] + "px";
            }
            check = true;
        } else if (animationPosition[i] > 0) {
            animationPosition[i] -= 2;
            menuElements[i].style.backgroundPosition="0px " + animationPosition[i] + "px";
            check = true;
        }
    }
    if (check) {
        setTimeout(doMove, 1); // call doMove in 20msec
    }
}

Context

StackExchange Code Review Q#37782, answer score: 3

Revisions (0)

No revisions yet.