Using a navigation menu to help keep material organized

Question

I'm working on a personal project to keep different snippets/examples/small projects of mine organized. I want to make the most of my page width, so I decided to write a navigation menu that slides out. It works as expected, but the code is kinda big.

I provided the HTML/CSS just in case that helps, but I'm really only looking for help with the JavaScript. I don't need anyone to rewrite the whole thing.

$(document).ready(function() {
	$('#tab').click(function() {
		if($('#main').attr('class') != 'out') {
			  $('#main').animate({
						'margin-left': '+=340px',
					  }, 500, function() {
						 $('#main').addClass('out');
					  });
												
				} else {
						$('#main').animate({
							'margin-left': '-=340px',
						  }, 500, function() {
							$('#main').removeClass('out');
						  });
						
					}
				});
				
				//does the same as above, but when I press ctrl + 1. from /http://www.scottklarr.com/topic/126/how-to-create-ctrl-key-shortcuts-in-javascript/
				$(document).keyup(function (e) {
					if(e.which == 17) isCtrl=false;
						}).keydown(function (e) {
							if(e.which == 17) isCtrl=true;
							if(e.which == 97 && isCtrl == true) {
								if($('#main').attr('class') != 'out') {
									  $('#main').animate({
												'margin-left': '+=340px',
											  }, 500, function() {
												 $('#main').addClass('out');
											  });
																		
										} else {
												$('#main').animate({
													'margin-left': '-=340px',
												  }, 500, function() {
													$('#main').removeClass('out');
												  });
												
											}
												return false;
											}
				});
			});

#sidebar { 	
	top: 0;
	left: 0;
	position: relative;
	float: left;
	}
	
#main {
	width: 320px;
	padding: 10px;
	float: left;
	margin-left: -340px;
	}

#tab {
	width: 30px;
	height: 120px;
	padding: 10px;
	float: left;
	}

.clear { clear: both; }

<div id="sidebar">
		<div id="main"></div>
		<div id="tab"></div>
		<br class="clear">
	</div>

Answer 1

First, it seems that isCtrl is not declared anywhere in your code. Put var isCtrl = false; as your first line inside the ready function. Otherwise, you will get a JavaScript error when the first key the user presses is not the Ctrl key. Also, == true is unnecessary within an if statement; it can be omitted.

Second, this line could be improved, as it fails if the element is a member of another class:

if($('#main').attr('class') != 'out') {

It can be rewritten using .hasClass() as:

if(!$('#main').hasClass('out')) {

In fact, you should refactor this entire block out to a separate function to avoid duplicating code:

function toggleMenu() {
    if(!$('#main').hasClass('out')) {
        // ...
    } else {
        // ...
    }
}

And put toggleMenu(); where the duplicate code was in each function. Also, if performance is a concern, you should cache the jQuery object $('#main') by declaring another variable at the beginning of the ready function. Not doing so is slow because jQuery would have to find the matched elements within the document again.

A more minor criticism is that of braceless control statements, such as:

if(e.which == 17) isCtrl=false;

Some say that it is best to always put the code within indented braces to avoid errors caused by incorrectly placed semicolons.

Finally, the indentation doesn't look right throughout the entire code. Fix that, and your code will become somewhat more readable.

To add: It also looks like you have an extra comma at the end of:

'margin-left': '+=210px',

This is syntactically valid, but it causes problems with Internet Explorer 7 and below.

Answer 2

idealmachine's advice is pretty sound. One more minor criticism, using if's to set boolean values is a bit redundant.

if(e.which == 17) { isCtrl = false; }

You can rewrite that a bit more elegantly as

isCtrl = e.which != 17;

Answer 3

You don't need to wrap toggleMenu in a function, you can just use

$('#tab').click(toggleMenu);

If you don't include the parenthesis, it's taken to mean a reference to the function rather than a function invocation.

It's a bad idea to try and manually keep track of keyboard modifiers. For instance, if your website looses focus while the key is held down, it will think it's still down when you return focus, even if it isn't.

The keyboard event has a ctrlKey property which will be true if the control key is being held down.

It's also probably a good idea to put the key code into a variable which describes which key it is, as having to remember what 97 maps to can be tricky.

$(document).keydown(function (e) {
    if (e.ctrlKey && e.which === aKey) {
        toggleMenu();
        return false;
    }
});

Lastly, it might be a good idea to extract the offset into variable.

var menuSize = '210px';
var $main = $("#main"); 

function toggleMenu() {
    if(!$main.hasClass('out')) {
        $main.animate({'margin-left': '+=' + menuSize}, 500).addClass('out');                         
    } else {
        $main.animate({'margin-left': '-=' + menuSize}, 500).removeClass('out');
    }
}