jQuery Accordion

Question

I have some JS that I am not OK with. How can I reduce the amount of code?

(function ($) {
 var allPanels = $('.accordion-modal > dd').hide();
 $('.firstlink').addClass('green');
 $('.firstlink > header').addClass('green');
 $('.accordion-modal > .active-accordian').show();
 $('.active-accordian').addClass('.active');
 $('.accordion-modal > dt > a').click(function () {
 $(this).children('header').addClass('green');
 $(this).parent().siblings().children('a').children('header').removeClass('green'); //$(this).parent().siblings().children('a').removeClass('green');
 allPanels.slideUp();
 $(this).parent().next().slideDown();
 return false;
 });
})(jQuery);

<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<dl class="accordion-modal">
 <dt><a href=""><header class='firstlink'>FIRST</header></a></dt>
 <dd class="active-accordian">FIRST CONTENT</dd>
<dt><a href=""><header>SECOND</header></a></dt>
<dd>SECOND CONTENT</dd>
</dl>

• Gary Storey: Was the answer below helpful? Any follow up?
• Miomir Dancevic: Txanks, but i still thinks it can be shorter :(

Answer 1

You could clean a lot of the code just by caching your selectors and chaining them where appropriate. This will improve performance as well. Also some of this code should be after the document.ready event to make sure the DOM elements are available to be manipulated. Dividing up your functionality into several smaller functions will help with debugging later on.

(function ($) {
 var modal, allPanels;
 function setActive( event ) {
 event.preventDefault(); 
 var $t = $(e.target); 
 modal.find('.green').removeClass('green'); //remove existing class
 $t.children('header').addClass('green');
 allPanels.slideUp();
 $t.parent().next().slideDown();
 }
 function init() {
 modal.find('.firstlink').addClass('green')
 .find('header').addClass('green').end().end()
 .find('.active-accordian').show().addClass('.active').end()
 .find('dt').on('click','a', setActive);
 }
 $(function() {
 modal = $('.accordion-modal');
 allPanels = modal.find('dd').hide();
 init();
 });
})(jQuery);

Answer 2

As you explained in the comments you are looking for something shorter. With that in mind, I completely re-wrote your javascript code to be much simpler and much more concise.

First off, if you can modify the HTML it would make life a lot simpler. To make an accordion, all your really need is this:

<dl class="accordion-modal">
 <dt>FIRST</dt>
 <dd>FIRST CONTENT</dd>
 <dt>SECOND</dt>
 <dd>SECOND CONTENT</dd>
</dl>

Then when you are ready to show the dd content, you can just do $(this).next().slideDown() to show the area when the dt is clicked.

FYI: I changed to a simple active class in the examples. Here is the adjusted code:

(function( $ ) {
 $(function() {
 var $acc = $('.accordion-modal');
 var $dd = $acc.find('dd');
 var $dt = $acc.find('dt');
 $dd.hide().first().addClass('active').show(); //hide all, show 1st, add class
 $dt.first().addClass('active'); //add active class
 $acc.on('click','dt', function() {
 $dd.slideUp().removeClass('active');
 $dt.removeClass('active');
 $(this).addClass('active').next().slideDown().addClass('active');
 });
 });
})( jQuery );

First we encapsulate the code in an IIFE, then on document.ready, we get are selections and add our click event.

Now, if you can't change the HTML, you can still use this code with some slight modification:

$(function(){
 var $acc = $('.accordion-modal');
 var $dd = $acc.find('dd');
 var $dt = $acc.find('dt'); 
 $dd.hide().first().addClass('active').show();
 $dt.first().addClass('active');
 $acc.on('click','header', function( event ) {
 event.preventDefault(); 
 $dd.slideUp().removeClass('active');
 $dt.removeClass('active');
 $(this).addClass('active').parents('dt').next().slideDown().addClass('active');
 });
});

The only real difference is we have to get the parent dt element first. I have created two fiddles. For the first option, you can use this fiddle and the second one is this fiddle.

Hope that helps.