The first thing you should do is encapsulate your code inside of an IIFE. This will create a private scope and help prevent pollution of the global namespace. Since you are using jQuery, you can pass it to the IIFE to make sure your reference to $ is always jQuery.
(function( $ ){
// your code here
})( jQuery );
The next thing would be to put anything that will need to manipulated the DOM inside of the document.ready event.
$(function(){
//code for once the DOM is available
});
The first few lines of your code have what are referred to as Magic Numbers.
var filterHeight = $('.product-filter').outerHeight() + 40; // 40 is M#
var sortbyHeight = $('.sortby').outerHeight() + 20; // 20 is M#
var speed = 1000; // is a magic number as well but already a variable
So what do 40 and 20 represent? I'm guessing its some sort of minimum height you want this to be. So create variables for them and comments to explain what they are:
var largeSize = 40; // minimum of 40px height
var smallSize = 20; // minimum of 20px height
var speed = 1000; // slide speed in milliseconds
var filterHeight = $('.product-filter').outerHeight() + largeSize;
var sortbyHeight = $('.sortby').outerHeight() + smallSize;
Next, you have these lines:
$('.product-filter').css('display', 'none');
$('.sortby').css('display', 'none');
Although that is perfectly valid, it is the same thing as .hide(); which is shorter to type if nothing else.
You definitely need to cache your selectors as well. Diving into the DOM to get the element each time is very resource intensive and one of jQuerys worst performing methods. So caching the selections into variables will increase the overall responsiveness of your page. For example:
if ($(this).hasClass('active')) {
if ($(this).hasClass('filter-header')) {
$('.clickable-header').removeClass('active');
$(this).addClass('active');
$('.sortby').slideUp(speed);
$(this).parent().find('.dropable-section').slideDown(speed);
$('#category-products').animate({ 'marginTop': filterHeight}, speed);
}
//rest of code
You could cache the selectors like this:
var $t = $(this),
$header = $('.clickable-header'),
$sortby = $('.sortby'),
$products = $('#category-products'),
$dropable = $t.parent().find('.dropable-section');
if ($t.hasClass('active')) {
if ($t.hasClass('filter-header')) {
$header.removeClass('active');
$t.addClass('active');
$sortby.slideUp(speed);
$dropable.slideDown(speed);
$products.animate({ 'marginTop': filterHeight}, speed);
}
Basically, if you are going to use it more than once, then it should be cached. If multiple functions need to access it, you should probably add it at the top level of your private scope so its available to all your functions. So the top of the code would look like this:
(function( $ ) {
var largeSize = 40; // minimum of 40px height
var smallSize = 20; // minimum of 20px height
var speed = 1000; // slide speed in milliseconds
var filterHeight, sortbyHeight, $productfilter;
var $header, $sortby, $products;
//rest of code
This will make these values accessible to all of our functions but, because of the IIFE, wont pollute the global scope. We should also create a function that will kick off everything.
function init() {
filterHeight = $('.product-filter').outerHeight() + largeSize;
sortbyHeight = $('.sortby').outerHeight() + smallSize;
$productfilter = $('.product-filter').hide(); //cache and hide
$sortby = $('.sortby').hide();
$header = $('.clickable-header');
$products = $('#category-products');
$header.on('click', headerClickHandler);
}
Then in document.ready call that function:
$(function(){
init();
});
The next step would be to DRY your code. The answer given by @atmd is a good step in this direction but it can be taken farther. It looks like there are two things your checking: 1) whether this is active and whether its the filter or the sort. So you can set up a few checks and remove all of the nested ifs. Taking the click event and making it a stand alone function (like above) will help with debugging of the code later. Here is a click handler function:
function headerClickHandler( ev ) {
var isActive, isFilter, marginTop,
$t = $(ev.target),
$dropable = $t.parent().find('.dropable-section');
$t.toggleClass('active'); // you always toggle class to start
isActive = !!( $t.hasClass('active') ); //returns true/false
isFilter = !!( $t.hasClass('filter-header') ); //returns true/false
//if its inactive then margintop is 0 otherwise get filter or sortby height
marginTop = (!isActive) ? 0 : (isFilter) ? filterHeight : sortbyHeight;
$header.removeClass('active');
$t.addClass('active');
$products.animate( { 'marginTop': marginTop }, speed );
$sortby.slideToggle(speed);
$productfilter.slideUp(speed);
$dropable[ (isActive) ? 'slideDown' : 'slideUp' ]( speed );
}
Much more concise.
I have updated your fiddle with the changes above. Also, in the fiddle I reduced the speed to 200 because I'm lazy and didn't want to wait that long :)
FIDDLE
Let me know if you have follow up questions or comments.
