My first-ever jQuery works, but it's not efficient. Any pointers?

Question

I've written my first completely self-written jQuery today. I just used the jQuery docs and I'm reasonably proud I got it to work, and working fine. It's not a very complex problem I'm solving, just an animated UI element, but it are my first steps, and I want to make sure I'm starting off right.

I think it's a lot of code for what I'm trying to achieve, and I'm looking for pointers or tips on how I could improve my code. I'm not at all averse to scrapping all jQuery here and doing it with pure JS either.

jQuery(document).ready(function () {
 event.preventDefault();
 var nav = jQuery(".navigation-menu");
 var button = jQuery(".navigation-title");
 function showNav() {
 nav.show('fast');
 button.removeClass('closed');
 button.addClass('open');
 }
 function hideNav() {
 nav.hide('fast');
 if (button.hasClass('open')) {
 button.removeClass('open');
 }
 button.addClass('closed');
 }
 hideNav();
 button.click(function () {
 if (button.hasClass('closed')) {
 showNav();
 } else if (button.hasClass('open')) {
 hideNav();
 } else if (!button.hasClass('open') || !button.hasClass('closed')) {
 hideNav();
 }
 });
});

Here's the fiddle: http://jsfiddle.net/yeawg/2/

I'm also wondering: am I using event.preventDefault() right?

Answer 1

I found .toggle() in the jQuery docs, and it seemed to me I could use that far better. I changed my code into:

jQuery(document).ready(function(){
 var nav = jQuery(".navigation-menu");
 var button = jQuery(".navigation-title");
 nav.toggle();
 button.click(function(event){
 event.preventDefault();
 nav.toggle('fast');
 });
});

This looks a lot leaner and better readable.

Answer 2

For one, it can be done CSS-only.

JS:

None at all. Remove your JS.

HTML:

<nav class="mobile-navigation">
 <!-- dummy target since CSS has no "prev" -->
 <i id="menu"></i>
 <div class="navigation-menu">
 <a id="menu-open" href="#menu">Menu</a>
 <a id="menu-close" href="#">Menu</a>
 </div>
 <ul id="menu-real" class="navigation-menu">
 <li><a href="#">Cafe</a></li>
 <li><a href="#">Toys</a></li>
 <li><a href="#">Shop-in-Shop</a></li>
 <li><a href="#">Feestjes</a></li>
 <li><a href="#">Agenda</a></li>
 <li><a href="#">Fietsverhuur</a></li>
 </ul>
</nav>

Add this CSS to your existing CSS:

/* menu animators */
#menu ~ #menu-real {
 height:0;
 width:0;
 opacity:0;
 box-sizing:border-box;
 position:absolute;
 overflow:hidden;
 transition: all 0.5s ease;
}
#menu:target ~ #menu-real {
 width: 100%;
 height : 100%;
 opacity : 1;
 position:relative;
}
/* Menu button swappers */
#menu ~ * > #menu-open {display:block}
#menu:target ~ * > #menu-open {display:none}
#menu ~ * > #menu-close {display:none}
#menu:target ~ * > #menu-close {display:block}

• Nick Rutten: Thanks for your answer, however I'm trying to learn more about JavaScript. I'm definitely looking in to your solution however. Seems the better option performance-wise. (Even though it probably won't matter much in this case, at least I don't have to load jQuery).

Answer 3

Some points:

• The preventDefault doesn't do anything in a ready event handler, as there is no default action that would happen after the event. (If you would use the preventDefault method you should declare a parameter in the event handler to catch the event object sent to it.)
• You don't need to check if an element has a class before removing it, if it's not there the removeClass method will just change nothing.
• You can chain calls that work with the same jQuery object.
• When checking what to do with the navigation, you can just use an else instead of checking every combination.

So:

jQuery(document).ready(function () {
 var nav = jQuery(".navigation-menu");
 var button = jQuery(".navigation-title");
 function showNav() {
 nav.show('fast');
 button.removeClass('closed').addClass('open');
 }
 function hideNav() {
 nav.hide('fast');
 button.removeClass('open').addClass('closed');
 }
 hideNav();
 button.click(function () {
 if (button.hasClass('closed')) {
 showNav();
 } else {
 hideNav();
 }
 });
});