patternjavascriptMinor
Navigation bar implementation
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
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;
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.
For example:
Here you are querying the DOM twice to find the
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.
Instead of
In the following code (and other similar code), the function (fadeInContact in this case) is called in every possible execution path:
In this case, the following is functionally equivalent and doesn't require you to put the function into every if/else branch:
You have three different functions which are identical with exception of the
All 3 of these could be replace by one with a parameter for the
Then it would be called like this:
While not 100% equivalent, making the assumption that only one of your elements is visible at a time, you can replace the following:
with
In fact,
With all changes above incorporated, the following:
Becomes:
- 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.
- 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.- 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();- Replace all
fadeInfunctions 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");- 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.