Grow your CSS skills. Land your dream job.

Case Study: jQuery Fixer Upper

Published by 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!

Comments

  1. Thank you for such a great article!

  2. What I strongly dislike is that if you clicked on “contact” and then click “web design” they both show. All should be hidden except the one that was clicked last.

    • DogsGhost

      You can just add an extra line before the toggle is called that hides all the IDs again.

      var content = ("#ContactArea, #WebDesign, #Portfolio, #AboutCoadin");
      
      $(content).hide();
      
      $("nav a").click(function(e){
        e.preventDefault();
        $(content).hide();
        $($(this).attr("href")).slideToggle();
      });
    • No no. I wasn’t needing help, just expressing my opinion.

    • I agree. But a case study loses value if you not only rewrite their code but rewrite the functionality.

  3. Very Interesting Chris.

    Thank you.
    I will take into account what you have said.

    But if you look at my website… http://coadin-internet.com
    you might agree with me that scrolling to the Divs (If possible in this situation) would be such a good idea.

    Thank you for taking the time to do a case study on my problem.

    Jonny.

    • Chuck Barlow

      A few suggestions: (1) Don’t use such a huge background image. 1.66 MB? At least make it a JPG if you can’t make it a smaller image that repeats. (2) Don’t make the buttons single-element lists with the text baked into the background image. That contributes nothing to semantics. Those backgrounds can easily be slivers repeated, but gradient CSS would be even better. (3) You put in the effort to make the page work with JavaScript disabled, so take it all the way. Don’t make the buttons act like buttons if they don’t do anything. (4) Instead of scrolling down, I think it would be better to move the buttons to one side and show the content on the other side, only showing one at a time. That keeps the scrollbars away and makes the animation more meaningful. Using Chris’s assumption that the buttons are in a single ul (bottom of “Select the appropriate element”) helps with this greatly, as well as with #2 and #3. (4.5) Capitalize Google and be consistent with your own name (“About Coadin” content vs. footer). Attention to details makes a difference.

    • If you don’t want to mess with making a repeating background image or change the buttons to CSS/repeating images you should at least run your page through GTMetrix and use the optimized versions of the images it provides (or Smushit or PunyPNG). Looks like that large background image can be losslessly compressed to 31.7KB

    • Another thing you could do:
      To prevent your content from jumping a tad to the left when clicking on one of your buttons because a scrollbar is added you could add: html { overflow-y: scroll}

      this will always add a scrollbar to the right side of the browser window (even if there is nothing to scroll), thus preventing above mentioned content jump.

      And a visual annotation, try adding some transparency to the white background of your content boxes (you can do this directly in css with rgba such as { background-color: rgba(255, 255, 255, 0.5) }, you can read more about that here: http://css-tricks.com/rgba-browser-support/)

    • Your form doesn’t have any labels nor placeholder text to inform the user which field requires which values. Also if you’re big into jQuery-ing your site, add some form validation to help prevent issues with XSS attacks and such. But not even having title=”” attributes to define each input field on hover is bad usability.

  4. WhiteInkDesign

    Nice write up Chris. Really showcases something I picked up last year at a conference about moving from writing strict declarative code to much more contextual coding.
    Often on places like forums where people are posting very declarative stuff and you don’t know their skill level, I usually don’t have the patience to try and help them with a problem and explain that there’s a smarter way to write their code. Now I can just point them to this post!

  5. There are 2 better reasons to use you version of the navigation.

    1. If javascript is disabled, you can still use the css :target selector to show or hide the sections.

    2. If javascript and css are disabled, e. g. in text browsers the page will still work as the user jumps to the wanted section.

  6. jack

    Don’t really like the flash of content. Could be prevented by a css ‘display:none’ right?

  7. Ron

    This type of post is what I’m always looking for. It’s one thing to understand the syntax of a language, and an entirely different thing to write really good code. There is a lot of information out there on how to write basic jQuery code, but not much that really differentiates why you would make the choices you made to make the code better. This is a great example of how to make working code better. Thank you!

  8. Being a fairly crappy-but-mostly-functional coder myself, I’d welcome more of these.

  9. Good tutorial Chris. One error though: class names and IDs can’t start with an uppercase character (e.g. #ContactForm should be #contactForm)

  10. arnold

    nice article more case study , please :) ….

  11. Great article, I learn something today :P

  12. Good article! As I was skimming down the initial code, my head was already making the same kinds of improvements you ended up making.

    I’ve recently started making heavy use of the pattern of grabbing the selector from an anchor tag within a nav element to manipulate the corresponding element with the matching ID. (Add a little string manipulation and/or DOM traversal, and the pattern also works well for getting to specific classes or other elements.) Your snippet for that looks pretty darned efficient.

  13. Brian Fegan

    One thing to note is this won’t work in IE7 as $(this).attr(“href”) will return the host, path, and any query string along with the id/hash.

    So you’d most likely want to do something like…

    $(“nav a”).click(function(e){
    e.preventDefault();
    var href = $(this).attr(‘href’);
    href = href.substring(href.indexOf(‘#’));
    $(href).slideToggle();
    });

  14. Nice! I would love to see more posts like this. I consider myself at an intermediate level with jquery/javascript and I knew a couple of these ideas but did not know about the others. I learned something today and that’s always good. Thanks Chris!

  15. Good article, these rules are very useful especially in big projects

  16. Ryan

    Awesome Chris. Great way to look into sample code and learn on how to optimize it with detailed explanations. I think it’s great to post things like this so we can all improve and push for better coding standards. Thanks again.

  17. Hassan

    Why not use Head JS (http://headjs.com/) instead of placing the script tag at the bottom for the sake of optimization?
    It also replaces the .no-js class on html element with .js (so no need for modernizr). It’s fantastic.

    • That’s cool, thanks for sharing. I think script loading is a whole big topic that is a little outside the scope of a little code cleanup, don’t wanna scare anybody off who’s trying to just pick up a few tips.

    • Been using head.js myself; lurving it!

  18. Garen

    Excellent work, I really like this article.

    It is actually exactly what i was looking for the small project i am working on.

    I have just broken down the code to fit into different areas of the page, it seems to be running fine on chrome/FF , but in IE (8) the #content boxes keeps resizing when ever you click on them.

    http://saassociates.webtodesigns.com/services.html

    (click on featured project(s))

    Any idea why?

  19. I don’t think you set the variables the way you meant to.

    This will ‘cache’ the selectors in a variable:


    var mySelector = $('#mySelector'),
    myOtherSelector = $('#myOtherSelector');

    And then you can re-use them:


    mySelector.hide();
    myOtherSelector.show();

    Can’t see a reason in this particular case to cache the hide function.

    I also could have sworn the preventDefault() needed to be called at the end of the custom function rather than the beginning.

    • I did, actually. I did it that way on purpose. jQuery’s chaining will return the same selector after .hide() is called on it. So there is no reason to set it to a variable and then call a function on it, you can combine into one statement!

    • Neato; I did not know that! Glad I asked!

    • (perhaps “asked” was the wrong word… glad I brought it up, I suppose is what I meant) ;-)

  20. thats the art of programming! you can make the code work, or you can make art ;)

    i love to compress scripts. when you can make 5 lines out of 15!

  21. hi,

    How can optimize the jquerry coding for search engine friendly…??? meet again.

  22. Wow really cool article I learnt loads here as a jQuery newbie. Maybe you could do this as a regular thing? It is great to see all the different solutions evolving here thank Chris and all the guys commenting.

    Cheers David

  23. Thanks for sharing, I did not know you could group elements like you did in the first line $( #xxxx, #aaaa, #bbb).hide()

  24. *cough*
    Actually, to answer Waffle’s question:
    $($(this).attr("href")).slideToggle("slow", function(){
    //scroll here
    });

    Though, since I’m no core JS ninja, I’d recommend using the following plugin for the actual scrolling (unless you’re a ninja already) http://plugins.jquery.com/project/ScrollTo

  25. Great read Chris. I did learn alot from this.

  26. Thank you, Chris! I learned a lot just by this little case study. I would love to see you do more of these, it’s a great idea!

  27. Gu

    Cant help cuz im newbie in jquery.. But the article is awesome! Keep doing that good job!

  28. Great article Chris!

    The funny thing about all this is that, the logic seems so straight forward and obvious. Coupling selectors similar to CSS and creating code so that it short and reusable (similar to OOPHP). Yet, I guess because I’m quite new to Javascript, I find myself making these careless mistakes. Thanks for this article. This is just what I needed to make my code more efficient.

    P.S. – (Once again a noob moment)

    Never knew to use e.preventDefault(); –> LOVE THIS.

This comment thread is closed. If you have important information to share, you can always contact me.

*May or may not contain any actual "CSS" or "Tricks".