A959afc1b4415ada875f4d3eaa889132

Hi, I am pretty new to JS, I have written this simple script for a menu and saving to a cookie using the jquery.Cookie plugin. It seems to work well but I'm sure it could be optimised (it seems pretty long). It would be great if someone could look over it for glaring mistakes and if anyone has any tips or tricks to optimise or write my code better, that would be great.

Thanks

$(document).ready(function(){
  $('#nav_menu_1').before('<a id="open_all" class="" href="#">Screen reader users can click here to open all menus</a>');
  $("#menu_column ul li").not("#menu_column ul li ul li").each(function(){
        checkCookie(this);    
    });
    
    function checkCookie(li) {
        var menu_index = $(li).index();
        var menu_id = $(li).parent().parent().attr('id');
        var cookie_state = $.cookie(menu_id + '_' + menu_index);
        
        if(cookie_state === null || cookie_state === '0') {
            $('ul', li).slideUp('fast');
            $("a", li).not("#menu_column ul li ul li a").addClass('closed'); // Add class of closed to top level headings to add rounded corners
            $("span", li).addClass('closed'); // Add class of closed to top level headings to add rounded corners
            if ($(li).children('ul').length) {
                $(li).children('a, span').before('<div class="toggle nav_arrow_light_down"></div>');
             } 
            else {
                $(li).not("#menu_column ul li ul li").addClass('no_toggle'); // If no children add no_toggle class
            }
        };
        
        if(cookie_state === '1') {
            $(li).children('a, span').before('<div class="toggle nav_arrow_light_up"></div>');
        };
    }
	
	 $('#menu_column #open_all').click(function(){ // For screen reader users - Opens all menus
        $('#menu_column .toggle').each(function(){
            if($(this).hasClass('nav_arrow_light_down')){
                $(this).siblings('ul').slideDown('fast');
                $(this).removeClass('nav_arrow_light_down');
                $(this).addClass('nav_arrow_light_up');
                $(this).next().removeClass("closed");
				changeCookie(this)
            }
        });
    });
    
    function changeCookie(toggle) {
        if($(toggle).hasClass('nav_arrow_light_down')){
                var menu_index = $(toggle).parent().index();
                var menu_id = $(toggle).parent().parent().parent().attr('id');
                $.cookie(menu_id + '_' + menu_index, '0', { path: '/' })
            }else{
                var menu_index = $(toggle).parent().index();
                var menu_id = $(toggle).parent().parent().parent().attr('id');
                $.cookie(menu_id + '_' + menu_index, '1', { path: '/' });
            }
    }
    
     $('.toggle').click(function(){
        $(this).siblings('ul').slideToggle(300);
        $(this).next().toggleClass('closed');
        $(this).toggleClass('nav_arrow_light_up');
        $(this).toggleClass('nav_arrow_light_down');
        changeCookie(this);
        return false
     });
    
});

Refactorings

No refactoring yet !

C541838c5795886fd1b264330b305a1d

naugtur

August 4, 2010, August 04, 2010 11:56, permalink

No rating. Login to rate!

I didn't break it down to see how it works, but there's a tip I can give You to optimize performance.
Repeating $(this) or $("anything") line by line is not what jQuery was intended for.

I've fixed up the bit from the end of Your code

[1] saves a lot of time on recreating the $(this) object
[2] this is called chaining and is the best feature of jQuery - You can call multiple methods on an object and if they are not supposed to return a value - they return the object so You can call some more.

$('.toggle').click(function(){
        var $this=$(this); // [1]
        $this.siblings('ul').slideToggle(300);
        $this.next().toggleClass('closed');
        $this.toggleClass('nav_arrow_light_up').toggleClass('nav_arrow_light_down'); // [2]
        changeCookie(this);
        return false
     });
A1ce281ac971bd75dc12349c40796b67

arkilus

August 24, 2010, August 24, 2010 23:31, permalink

No rating. Login to rate!

Regarding chaining, this should work:

$(this).siblings('ul').slideToggle(300).end()
       .next().toggleClass('closed').end()
       .toggleClass('nav_arrow_light_up')
       .toggleClass('nav_arrow_light_down');
Bf0c2ee31aee9e20f0b107f2782a66f0

Fadzril

December 7, 2010, December 07, 2010 14:24, permalink

No rating. Login to rate!

for selector:.toggle we can use chaining method like posted previously. Please correct if i'm wrong..

function changeCookie(toggle) {
    var element = $(toggle),
        menu_index = element.parent().index(),
        menu_id = element.parent().parent().parent().attr('id') //better if we can select parents(selector)

    (element.hasClass('nav_arrow_light_down')) ?
        $.cookie(menu_id + '_' + menu_index, '0', { path: '/' }) :
        $.cookie(menu_id + '_' + menu_index, '1', { path: '/' }) ;
}
    
$('.toggle').click(function() {
    var element = $(this);
    element
        .siblings('ul')
        .slideToggle(300)
        .andSelf()
        .next().toggleClass('closed')
        .andSelf()
        .toggleClass('nav_arrow_light_up')
        .andSelf()
        .toggleClass('nav_arrow_light_down');
    
    changeCookie(element)
    return false;
});

Your refactoring





Format Copy from initial code

or Cancel