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

Menu script efficiency

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

Problem

I've got a jQuery/JavaScript script for a menu that uses icons as well as label text. When the icon or text is clicked, it turns blue, until such time as another icon is clicked.
Also, the page scrolls down to the relevant div on the page, but the menu bar still stays fixed along the top of the screen.

I've created several versions of this script and this is the most efficient (least code) I have managed so far.

Can anyone tell me any methods to use instead, or any other way I could make it more efficient?

```
window.onload = function() {

[].forEach.call(document.querySelectorAll('.navico'), function(el) {
el.addEventListener('click', imageButtonClickHandler);
});

function imageButtonClickHandler()
{
switch(this.id)
{
case "aboutnav":
grey();
$('#abouttitle').css('color', 'blue');
$('#a').css('color', 'blue');
$('html, body').animate( { scrollTop: $("#about").offset().top - 90 }, 2000);
break;
case "routenav":
grey();
$('#routetitle').css('color', 'blue');
$('#b').css('color', 'blue');
$('html, body').animate( { scrollTop: $("#route").offset().top - 90 }, 2000);
break;
case "enternav":
grey();
$('#entertitle').css('color', 'blue');
$('#c').css('color', 'blue');
$('html, body').animate( { scrollTop: $("#enter").offset().top - 90 }, 2000);
break;
case "racedaynav":
grey();
$('#rac

Solution

I am happy you posted your best script, you can still see a ton of repetition here.. If you were to always name the navbutton after the scroll target and title ( "aboutnav" -> about & abouttitle ) then this can be much shorter. For now I can only see resultsnav break the mold.

I would suggest something like this :

window.onload = function () {

    [].forEach.call(document.querySelectorAll('.navico'), function (el) {
        el.addEventListener('click', imageButtonClickHandler);
    });

    function imageButtonClickHandler() {
      var buttonId = this.id,
          target   = buttonId.substring( 0 , buttonId.length - 3 ), 
          titleId = '#' + target + 'title',
          scrollTarget = '#' + target;

      var navMap = {
          'aboutnav'   : '#a',
          'routenav'   : '#b',
          'enternav'   : '#c',
          'racedaynav' : '#d'
          //etc. etc.
      }    

      grey();
      $( titleId ).css('color', 'blue');
      $( navMap[ target ] ).css('color', 'blue');
      $('html, body').animate({
        scrollTop: $(scrollTarget).offset().top - 90
      }, 2000);      
   }
};

function grey() {
    $('.sectitle').css('color', 'grey');
    $('.roger').css('color', 'grey');
    return;
}


Basically, I derive the link id, the scroll target id from the name of the nav link. Then, I also mapped the nav names to 'a','b','c' etc. From there this becomes a really short piece of code. I also changed the tabs to 4 spaces, if you use more than 4 spaces, then it becomes harder to grok the code and realy understand what you are doing. I also opted for single quote strings, easier to read and less carpal tunnel by hitting shift all the time ;)

Note that you could write grey like this:

function grey() {
    $('.sectitle, .roger').css('color', 'grey');
}


You can comma chain selectors, also the function will return regardless at the end, you dont need to put return.

Code Snippets

window.onload = function () {

    [].forEach.call(document.querySelectorAll('.navico'), function (el) {
        el.addEventListener('click', imageButtonClickHandler);
    });

    function imageButtonClickHandler() {
      var buttonId = this.id,
          target   = buttonId.substring( 0 , buttonId.length - 3 ), 
          titleId = '#' + target + 'title',
          scrollTarget = '#' + target;

      var navMap = {
          'aboutnav'   : '#a',
          'routenav'   : '#b',
          'enternav'   : '#c',
          'racedaynav' : '#d'
          //etc. etc.
      }    

      grey();
      $( titleId ).css('color', 'blue');
      $( navMap[ target ] ).css('color', 'blue');
      $('html, body').animate({
        scrollTop: $(scrollTarget).offset().top - 90
      }, 2000);      
   }
};


function grey() {
    $('.sectitle').css('color', 'grey');
    $('.roger').css('color', 'grey');
    return;
}
function grey() {
    $('.sectitle, .roger').css('color', 'grey');
}

Context

StackExchange Code Review Q#51511, answer score: 3

Revisions (0)

No revisions yet.