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 its 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 be 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.
IDs
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 IDs 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:
- 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.
- 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.
- 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();
});
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!
Thank you for such a great article!
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.
You can just add an extra line before the toggle is called that hides all the IDs again.
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.
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.
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: https://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.
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!
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.
Don’t really like the flash of content. Could be prevented by a css ‘display:none’ right?
See Doug’s comment below for solution on this.
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!
Agreed. I love this. And sidenote, I love the details you’ve put into this comment form.
Great job Chris, the code looks great. If you wanted to take it a step further (not sure if its really needed), but I would do two other things. One is, I would use “js” and “no-js” classes to take care of the hiding of the initial elements. If your code truly runs at the bottom of the page, you stand a high risk of seeing a flash of the content before jQuery has a chance to hide it:
Keep in mind if you are using Modernizr, all you have to do is include the `no-js` class, the swap of the classes is taken care of by Modernizr.
Then, in the CSS:
That removes the need to select hide the elements. Then, the last thing I would do is use Delegate, so absolutely no DOM traversal occurs until a user clicks a button. That means, the only jQuery needed to manage this is the just the delegate command as the initial hiding is handled by the CSS:
This is similar to just using “live”, but in this case, we attach the handler directly to
document
instead of first making a selection (The way .live works) since our code is running at the bottom of the page anyway.a really good solution…well done
Something to note here is that when we use display:none or any of jQuery’s hiding methods (hide, slideUp, fadeOut), it makes the content inaccessible for screen readers. If you’re looking to keep the content screenreader-friendly I’d recommend toggling a “hidden” class and using the CSS to position the element off screen.
It will also help improve the performance of your script since addClass(), removeClass() is a little faster than hide(), show().
You can still use the animations, but be sure to remove the style:display:none that jQuery adds to the element and replace it with your own class.
I think $(this).attr(“href”) can be written as this.href which is faster.
In most modern browsers yeah I think this.href is fine, but I think there are some IE 7- issues with that maybe? jQuery does some normalization on that to make sure it’s consistant. I could be wrong though, I know this.id has very deep browser support and almost always the best way to go.
I’ve often wondered when delegate is appropriate over click. In my mind I figured delegate when the DOM will change, click when it won’t.
I get the bubbling part, but for some reason I thought delegate would take more juice to monitor the events. Totally off-base? Always delegate when it’s a structural thing like li > a or nav > a?
And followup:
why
$(document).delegate(‘nav a’, etc)
instead of
$(‘nav’).delegate(‘a’, etc)
I had a similar reaction after reading this article: why not hide everything initially with display:none, saving yourself a line of javascript, and probably a flash of content?
So I scrolled through the comments to see if any one had posted that before I posted it myself. When I read yours, it made sense, but it suddenly dawned on me why this solution is bad…
From everything that I have read, screen readers never speak stuff that is hidden with display:none. Screen readers also do not fire javascript. Which means that by hiding the content with CSS the way you (and I) thought to do, you are making that content invisible to blind people.
Just something to think about.
Sean, if I’m not mistaken the elements won’t be hidden on screen readers. As the screen reader doesn’t fire javascript the class of the html element will remain as ‘no-js’ and subsequently not be hidden by the css rules (as they only target html.js’).
Being a fairly crappy-but-mostly-functional coder myself, I’d welcome more of these.
me too, and me too.
Good tutorial Chris. One error though: class names and IDs can’t start with an uppercase character (e.g. #ContactForm should be #contactForm)
Huh? According to whom? HTML allows this. IDs must start with an alphabetic character (case-insensitive), and class names can be pretty much anything: https://css-tricks.com/unicode-class-names/.
In html5, they are even more relaxed and can contain basically any characters but spaces. Makes it easy to have IDs like “/about” that match URLs.
nice article more case study , please :) ….
Great article, I learn something today :P
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.
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();
});
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!
Good article, these rules are very useful especially in big projects
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.
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!
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?
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) ;-)
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!
Well said Nico…
hi,
How can optimize the jquerry coding for search engine friendly…??? meet again.
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
Thanks for sharing, I did not know you could group elements like you did in the first line $( #xxxx, #aaaa, #bbb).hide()
*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
Great read Chris. I did learn alot from this.
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!
Cant help cuz im newbie in jquery.. But the article is awesome! Keep doing that good job!
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.