Case Study: jQuery Fixer Upper

Chris Coyier //

Recently in the CSS-Tricks Forums, user named Waffle posted the following bit of jQuery JavaScript:

$(function(){

	$('.ContactArea').hide();
                
	$('.Portfolio').hide();
                
	$('.WebDesign').hide();

	$('.AboutCoadin').hide();

	$('li.Contact').click(function(){
		$(".ContactArea").slideToggle();
	});

	$('li.PortfolioBtn').click(function(){
		$(".Portfolio").slideToggle();
	});

	$('li.WebDesignBtn').click(function(){
		$(".WebDesign").slideToggle();
	});

	$('li.AboutBtn').click(function(){
		$(".AboutCoadin").slideToggle();
	});
                        
});

This user's actual question was how they can extend their code to add functionality to scroll the page down to the areas as they expand. But in looking at code like this with a slightly more experienced eye, there are things that stick out as things that we can likely improve upon. Since I bet there are some of you folks that are at Waffle's level and writing code like this, I thought we could use this as a case study to learn from.

It works, but...

To start with, Waffle's code isn't broken, it works fine and because of it's simple nature the improvements that we'll end up making probably won't even make noticeable speed improvements. However, the improvement we make should make the code more readable, more efficient, more extensible, and improve other related things. Also, understanding the improvements we make will make us better coders in the future when the stakes are higher.

Do we need a DOM ready statement?

The first line of code is this:

$(function(){

...which is jQuery shorthand for "When the DOM is ready to manipulated, run this function..." This is a safety net so that, for example, selectors aren't run on HTML that isn't parsed yet, leaving us wondering WTF. It also doesn't wait until the page has fully loaded, which would make our script appear sluggish.

But the thing is, we should be running scripts like this at the bottom of our pages (before the closing </body> tag), so that loading the script doesn't hold up page rendering. If we load scripts properly at the bottom of the page, a DOM ready statement isn't needed, as the browser has dealt with all that HTML before it has even started reading the script.

Multiple Selectors, One jQuery Object

The next lines of code are:

$('.ContactArea').hide();             
$('.Portfolio').hide();     
$('.WebDesign').hide();
$('.AboutCoadin').hide();

Creating four separate jQuery objects here is unnecessary, as you can use multiple selectors in one. This does the same thing:

$(".ContactArea, .Portfolio, .WebDesign, .AboutCoadin").hide();

Just like a CSS selector. However, you might actually want four separate jQuery objects, so that you can refer to them later without needing to create them new. In that case, set variables:

var contactArea = $('.ContactArea').hide(),
    portfolio   = $('.Portfolio').hide(),
    webDesign   = $('.WebDesign').hide(),
    about       = $('.AboutCoadin').hide();

You might also consider adding a class name to the HTML like "initial-hide" and applying it to all elements you wish to hide at time of page load and selecting by that, but that may walk on the wrong side of the semantics line.

ID's

It's hard to say without seeing the real website, but I have a feeling that these areas like "contact area" and "portfolio" are totally unique areas on the page, not things that are repeated more than once. Totally unique areas are ideal for using ID's (as opposed to classes). The benefit here being that ID selectors are faster.

$("#ContactArea") // is faster than
$(".ContactArea")

The other important things about ID's is that they are natural jump-links, which work regardless of JavaScript being enabled:

<!-- This link will jump down the page ... -->
<a href="#ContactArea">Contact</a>

<!-- ... to ensure this element is visible -->
<section id="ContactArea"></section>

You may not actually want the page to jump down, but you can stop that with JavaScript by preventing default behavior. Without JavaScript, you definitely want same-page navigation to be jump links.

Select the appropriate element

The next bit of code has four repetitive bits:

$('li.Contact').click(function(){
	$(".ContactArea").slideToggle();
});

$('li.PortfolioBtn').click(function(){
	$(".Portfolio").slideToggle();
});

$('li.WebDesignBtn').click(function(){
	$(".WebDesign").slideToggle();
});

$('li.AboutBtn').click(function(){
	$(".AboutCoadin").slideToggle();
});

The first thing to discuss there is the actual element being selected. Presumably, the markup is something like this:

<nav>
  <ul>
     <li class="ContactArea"><a href="#">Contact</a></li>
     <li class="PortfolioBtn"><a href="#">Contact</a></li>
     <li class="WebDesignBtn"><a href="#">Contact</a></li>
     <li class="AboutBtn"><a href="#">Contact</a></li>
  </ul>
</nav>

The jQuery code is selecting the list items by their class names. These are my issues with that:

  1. They don't all use the "Btn" convention. If you stick with classes like this, you might as well make it standard. Not sticking to it in this tight of quarters would be symptomatic of other, larger inconsistencies.
  2. You don't need the classes for CSS to style them uniquely. You can use :nth-child or :nth-of-type. Unless you need to support super old browsers, in which case, you are using jQuery anyway and could feign support with that.
  3. Because you don't need them for styling, I'd say you don't really need them at all.

Assuming the markup can be changed to use ID's instead of classes, the HTML could be rewritten like this:

<nav>
  <ul>
     <li><a href="#ContactArea">Contact</a></li>
     <li><a href="#Portfolio">Portfolio</a></li>
     <li><a href="#WebDesign">Web Design</a></li>
     <li><a href="#AboutCoadin">About</a></li>
  </ul>
</nav>

This is a more semantic representation of navigation. No unneccesary classes and anchor links that are same-page links to the appropriate content.

Don't Repeat Yourself

Which is commonly referred to as "DRY." The four statements that do all that selecting and slideToggling are very repetitive. We can improve upon that with our new and improved HTML structure.

The href attribute for those anchor links in the navigation are like "#Contact", which is exactly what we need as a selector string to find that element. Super convenient! Let's yank that href from the link and use it as a selector, that way one single click handler can handle all four of our links:

$("nav a").click(function(e){
  e.preventDefault();
  $($(this).attr("href")).slideToggle();
});

Final Code

Our final code ends up being much more succinct:

$("#ContactArea, #WebDesign, #Portfolio, #AboutCoadin").hide();

$("nav a").click(function(e){
  e.preventDefault();
  $($(this).attr("href")).slideToggle();
});

View Demo

To actually answer Waffle's question

The posed question was actually "I also want the page to scroll to those divs when those buttons are pressed" To which I would say, are you sure? That seems like a weird combination of animated activity. Smooth scrolling same-page links are actually a little more complicated than you might think, but I have some great copy-and-pasteable code for them here. The problem is that it won't quite work in this scenario, because at the time the link is clicked the element to scroll to is actually hidden. It's not until the slideToggle is done where the element is unhidden and at it's final size. You'd have to delay the page scroll until the callback function for that. Doable, but janky.

Did I do OK?

One of my favorite parts about publishing blog posts on this site is that people really take the time to read it and offer quality suggestions. If you have some even better ways at handling the things I suggested above, I'd love to hear it!