Jquery accordion

[falkencreative]

$("#nav4").click(function() { 

// check if current active item. If so, do nothing
if ($(this).hasClass("active")) { return false; }

// clear all currently active subnav items
$("a.active").removeClass("active");
// make this clicked item currently active
$(this).addClass("active");

// hide current active section                          
$("h3.active").next("div.show").animate({
opacity : 'toggle',
height: 'toggle'
}, 'slow');
$("h3.active").removeClass("active");

// show new active
$("#section4").next("div.show").animate({
opacity : 'toggle',
height: 'toggle'
}, 'slow');

// set as active        
$("#section4").addClass("active"); 
return false;
});

 

I know enough jQuery to get things working, but unfortunately don't always make very pretty code. There has to be a better way to do this.

 

Here's the setup: I have a subnavigation. Each of the "a" elements in the subnav have id's associated with them ("nav1", "nav2" etc). When clicked, I want to have a section of the accordion in the main body of the page fade/slide into view. Each of those main accordion sections have id's ("section1", "section2" etc) on the "h3" header, and the each have a div below the h3 with a class of "body" with content.

 

The issue is, I'm repeating the above code for each element in the subnavigation. Is there any way to simplify things, so I can use one block of code for this functionality, rather than multiple blocks?

 

Example: http://falkendev.com/kirshner/sub.html (I haven't yet checked the site in IE, so there may be issues. It should work/look fine in Firefox/standards based browsers)

[monkeysaurus]

Hi Ben,

 

I feel a long-ish answer coming on!

 

My first suggestion would be that in re-creating a jQuery accordion, you're re-inventing the wheel. jQuery UI provides an accordion widget that is functional cross-browser, keyboard-accessible, theme-able, has support for WAI-ARIA and much, much more. So unless you're planning to implement all of those features, this is almost certainly a better option for your client, and their customers.

 

It is as simple to implement as:

 

<script type="text/javascript">
$(function() {
	$("#accordion").accordion();
});
</script>

 

The only time when it's appropriate to reinvent the wheel (IMHO) is when you're trying to learn about wheels. So having posted the above advice, I'll offer some suggestions as to the code you posted and the questions you actually asked.

 

My first piece of advice would be to re-organise your code as a jQuery plugin. This is really easy to do, and encourages you to re-use code you've written in the past. A full tutorial on plugin authoring is more than I want to get into here, but there is plenty of information on jQuery plugin authoring on their website.

 

I have put together the very simplest of accordions on jsfiddle.net to demonstrate how writing plugins keeps your code clean and re-usable. The flip side is that your HTML has to be what the plugin expects, so markup needs to be the same on subsequent usages.

 

The other down-side is that if your markup changes in the future, you'll have to test thoroughly to make sure the accordion still works. This is a job for a unit testing tool like FireUnit, but that's for another post.

 

A couple of other points. Using classes of .nav instead of #nav1, #nav2, #nav3 and #nav4 means that you can attach behaviour once. When you do this, use DOM traversal functions such as next(), previous(), parent(), children() and find() to find the related DOM elements rather than referring to specific divs in each anonymous function. (For example, steer clear of things like hard-coded $('#section4'))

 

Once you've done this, you can get rid of 3/4 of the code! (Or if there were 10 accordion items, 9/10ths!) You'll notice that I have done this in the jsfiddle example I posted above.

 

Another thing to note too. (Not strictly related to your question but it's a pet peeve of mine!)

 

If you find yourself calling $(this) or $('#mySelector') in multiple places within a given code block, this can almost certainly be optimised. Every time you call the jQuery function (which you are doing when you refer to $(this)), you incur an unnecessary performance hit. Function calls in JavaScript are expensive, and should be avoided if possible. In addition, if you call $('#selector') more than once, you are calling the jQuery function and traversing the DOM to find the element; not good if it is a complex selector or there are many elements in your page.

 

To get around this, you can do something like the following. Rather than:

 

$('.foo').hide();
$('.foo').show();
$('.foo').animate(...);

 

You can cache your selectors by doing the following:

var $foo = $('.foo');
$foo.hide();
$foo.show();
$foo.animate(...);

 

We've assigned the elements to a variable which has had the jQuery function applied to it, and referred to the variable later in the code. You can see in the example above that we've saved 2 function calls and 2 DOM traversals at the cost of one line of code! I like to put a $ sign before the variable name to remind me that the elements have been 'jQuery-fied', but that's just a personal convention.

 

Hope that makes sense, please ask if you'd like me to clarify anything.

Edited August 18, 2010 by monkeysaurus

[falkencreative]

Just a quick reply regarding using the jQuery UI Accordion plugin... I'm not sure if that does what I want, which is why I didn't go with it originally. If you take a look at the sample link I posted, I want to be able to click subnavigation items in the left column and have the correct navigation item open (ideally without page refresh) in addition to being able to open/close accordion items by clicking on the main header.

 

I'm still going over the rest of the post, and thanks for taking a look.

[monkeysaurus]

Apologies Ben, I completely missed that part of the requirements.

 

Yes, it is entirely possible to do this with jQuery UI.

 

BEHOLD!

Edited August 18, 2010 by monkeysaurus

[BeeDev]

// Get all anchors with ID starting with nav
var $nav = $("a[id^=nav]");
// Get all h3 with ID starting with section
var $sec = $("h3[id^=section]");

$nav.each(function(i){
var $this = $(this);
$this.bind("click",function(e){
	if(!$this.hasClass("active")){
		// cancel default anchor click event behavior ...
		// ... same as "return false" but doesn't stop the further execution of code
		e.preventDefault();

		// clear all currently active subnav items & activate current subnav item
		$nav.removeClass("active");
		$this.addClass("active");

		// hide current active section and remove active class
		$("h3.active").removeClass("active").next("div.show").animate({
			opacity : 'toggle',
			height: 'toggle'
		}, 'slow');

		// show the section that was clicked on
		$sec.eq(i).addClass("active").next("div.show").animate({
			opacity : 'toggle',
			height: 'toggle'
		}, 'slow');
	}
});
});

 

notice the variable 'i' being passed to the each function. This has the counter starts from 0. And this counter is being used in the last part to reference the correct #sectionX item:

 

// show the section that was clicked on

$sec.eq(i).next("div.show").animate({

opacity : 'toggle',

height: 'toggle'

}, 'slow').addClass("active");

 

.eq() function's name is an abbreviation of "equals" i'm guessing here. Basically if you pass it a number, it will select the n'th item from the object array.

 

$sec.eq(i) will select the 2nd item if i=1, and i=1 will only be true if the 2nd link (#nav) is clicked on.

 

I haven't tested this code but it should work, if not I trust you can debug it yourself

 

And why use a 15kb Javascript library when you can achieve the same result with 1kb?

var $nav=$("a[id^=nav]");var $sec=$("h3[id^=section]");$nav.each(function(i){var $this=$(this);$this.bind("click",function(e){if(!$this.hasClass("active")){e.preventDefault();$nav.removeClass("active");$this.addClass("active");$("h3.active").next("div.show").animate({opacity:'toggle',height:'toggle'},'slow').removeClass("active");$sec.eq(i).next("div.show").animate({opacity:'toggle',height:'toggle'},'slow').addClass("active");}});});

(minified with http://www.ventio.se/tools/minify-js/ )

Edited August 19, 2010 by BeeDev

[dianikol85]

Did you solve it Ben? i just look the example and the nav on the right do as you want

[falkencreative]

Thanks everyone for the help. BeeDev, that's exactly what I was looking for, though I haven't played with the code yet to see if it works. I figured there was some way to use an array/loop, rather than having individual click events for each item.

 

John, thanks for the code as well. I'm not sure if I want to go the jQuery UI way, at least, not if I'm not using the library elsewhere in the project, but I'll see if there is a way to convert this into a plugin for future use.

[BeeDev]

You're welcome.

 

I guess a small correction would be to move e.preventDefault() outside the "if statement" as it should run everytime regardless if the link is the active one or not.

 

// Get all anchors with ID starting with nav
var $nav = $("a[id^=nav]");
// Get all h3 with ID starting with section
var $sec = $("h3[id^=section]");
// Declare $this
var $this;

$nav.each(function(i){
       $this = $(this);
       $this.bind("click",function(e){
               // cancel default anchor click event behavior ...
               // ... same as "return false" but doesn't stop the further execution of code
               e.preventDefault();

               if(!$this.hasClass("active")){                        
                       // clear all currently active subnav items & activate current subnav item
                       $nav.removeClass("active");
                       $this.addClass("active");

                       // hide current active section and remove active class
                       $("h3.active").removeClass("active").next("div.show").animate({
                               opacity : 'toggle',
                               height: 'toggle'
                       }, 'slow');

                       // show the section that was clicked on
                       $sec.eq(i).addClass("active").next("div.show").animate({
                               opacity : 'toggle',
                               height: 'toggle'
                       }, 'slow');
               }
       });
});

 

There's also a way to avoid the weird bouncing effect that results from resize 2 divs at the same time. It's a little bit complicated to explain, but basically the jQuery.animate() function has few callbacks, and one of them is called "step". You can basically assign a function to this which gets fired on every step of the animation. This way you make sure that when div.a reduces by 2px div.b increases by 2px. But use .css() instead of .animate() inside the step() callback. Anyway probably this level of careful animation is not needed

Edited August 19, 2010 by BeeDev