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

Navigation bar implementation

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

Problem

I made a navigation bar on the left side of my site. It works perfectly, the only problem is I'm new to jQuery and I think I'm repeating too much code. Is this the case? If it is please help me out.

HTML


DopEYEmine

    
        
            
            ABOUT
            SUBMIT
            CONTACT
        
    

    
        
            CLOSE
            
                
            
            "Dopamine: the molecule behind all our most sinful behaviors and secret cravings."
            
        
    

    
        
            CLOSE

            
                
                
            

        
    

    
        
            CLOSE
        
    

    

    


CSS

```
html, body {
height: 100%;
width: 100%;
overflow: hidden;
}

a{
text-decoration: none;
}

/Side Bar Nav----------------------------------/

.logo {
position: absolute;
top: 30px;
left: 10px;
width: 75px;
margin: 0px;
}

.logotwo {
position: absolute;
top: 34px;
left: 110px;
width: 150px;
display: none;
}

#sidebar {
float: left;
z-index: 10;
position: relative;
top: 0;
left: 0;
height: 100%;
width: 95px;
background: #555;
overflow: hidden;
border-right: solid 3px #444;

-webkit-transition: all 0.5s ease;
-moz-transition: all 0.5s ease;
-ms-transition: all 0.5s ease;
-o-transition: all 0.5s ease;
transition: all 0.5s ease;
}

#sidebar:hover {
width: 285px;
background: #000;
border-right: none;
}

.nav_icons {
width: 40px;
height: 40px;
}

#sidebar li {
display: block;
font-family: 'Special Elite', cursive;
}

#sidebar ul li:nth-child(2) {
position: absolute;
top: 88px;
left: 27px;
}

#sidebar ul li:nth-child(3) {
position: absolute;
top: 148px;
left: 27px;
}

#sidebar ul li:nth-child(4) {
position: absolute;
top: 208px;
left: 27px;
}

.about_btn,
.submit_btn,
.contact_btn {
position: relative;

Solution

I have a few suggestions.

  1. Always cache your jquery objects if you are going to select the same thing more than once.



For example:

if($('#submit').css("display") == "block") {
    $('#submit').fadeOut(10);


Here you are querying the DOM twice to find the #submit element. Instead, save it to a variable and use that variable in both places:

var $submit = $('#submit');

if($submit.css("display") == "block") {
    $submit.fadeOut(10);


In fact, assuming the elements in question are not dynamically created/removed, you can add this variable assignment at the very top of your code (inside your document.ready) and use it throughout your code. This is only 1 single query instead of multiple with every click.

  1. For code readability use .is(":visible") to check if an element is visible.



Instead of $submit.css("display") == "block", which I am assuming you are using to check to see if the element is display: none (hidden) vs display: block (visible), you can use $submit.is(":visible"). This is slightly more concise but more importantly better conveys what the code is doing.

  1. If a function is called in all possible execution paths, call it after the if



In the following code (and other similar code), the function (fadeInContact in this case) is called in every possible execution path:

if($('#about').css("display") == "block") {
    $('#about').fadeOut(10);
    fadeInContact();
} else if($('#submit').css("display") == "block") {
    $('#submit').fadeOut(10);
    fadeInContact();
} else {
    fadeInContact();
}


In this case, the following is functionally equivalent and doesn't require you to put the function into every if/else branch:

if($('#about').css("display") == "block") {
    $('#about').fadeOut(10);
} else if($('#submit').css("display") == "block") {
    $('#submit').fadeOut(10);
}

fadeInContact();


  1. Replace all fadeIn functions with just one.



You have three different functions which are identical with exception of the id of the element. One example:

var fadeInContact = function() {
    $('#contact').animate({width: 'toggle'});
};


All 3 of these could be replace by one with a parameter for the id:

var fadeInElement = function(id) {
    $('#' + id).animate({width: 'toggle'});
};


Then it would be called like this:

fadeInElement("contact");


  1. Use chaining and filters to consolidate code.



While not 100% equivalent, making the assumption that only one of your elements is visible at a time, you can replace the following:

if($('#about').css("display") == "block") {
    $('#about').fadeOut(10);
} else if($('#submit').css("display") == "block") {
    $('#submit').fadeOut(10);
}


with

$("#about, #submit").filter(":visible").fadeOut(10);


In fact, fadeOut will implicitly do a check for visibility and only fade out if the element is visible. As a result, the filter can be excluded making it simply:

$("#about, #submit").fadeOut(10);


With all changes above incorporated, the following:

$(".about_btn").click(function () {
    if($('#submit').css("display") == "block") {
        $('#submit').fadeOut(10);
        fadeInAbout();
    } else if($('#contact').css("display") == "block") {
        $('#contact').fadeOut(10);
        fadeInAbout();
    } else {
        fadeInAbout();
    }
});


Becomes:

$(".about_btn").click(function () {
    $("#contact, #submit").fadeOut(10);

    fadeInElement("about");
});

Code Snippets

if($('#submit').css("display") == "block") {
    $('#submit').fadeOut(10);
var $submit = $('#submit');

if($submit.css("display") == "block") {
    $submit.fadeOut(10);
if($('#about').css("display") == "block") {
    $('#about').fadeOut(10);
    fadeInContact();
} else if($('#submit').css("display") == "block") {
    $('#submit').fadeOut(10);
    fadeInContact();
} else {
    fadeInContact();
}
if($('#about').css("display") == "block") {
    $('#about').fadeOut(10);
} else if($('#submit').css("display") == "block") {
    $('#submit').fadeOut(10);
}

fadeInContact();
var fadeInContact = function() {
    $('#contact').animate({width: 'toggle'});
};

Context

StackExchange Code Review Q#48839, answer score: 2

Revisions (0)

No revisions yet.