7
\$\begingroup\$

The following works as intended, however is there a more concise way to write this script without changing it too much? It's a simple menu with sub-levels which have a plus/minus icon when each li opens/closes.

jQuery:

$(document).ready(function() {
    $li = $('#main > li.expand');
    $li.click(function(){
        $ul = $(this).find('ul.sub-level').delay(200).slideDown(400);
        $(this).removeClass('expand').addClass('minus');
        $('#main > li > ul').not($ul).slideUp('slow');
        $('#main > li.minus').not($(this)).removeClass('minus').addClass('expand');

    })

    $li2 = $('.sub-level > li.expand');
    $li2.click(function(){
        $(this).find('ul.sub-level2').delay(200).slideToggle(400);
        $li2.toggleClass('minus').toggleClass('expand');
    })      
});

HTML:

<ul id="main">
<li class="expand">test-1
<ul class="sub-level">
    <li class="expand">sub-test-1
        <ul class="sub-level2">
            <li>sfasfasf</li>
            <li>sfafasf</li>
        </ul>
    </li>
  </ul>
  </li>
  <li>test-2</li>
  <li class="expand">test-3
  <ul class="sub-level">
    <li>sub-test-2</li>
    </ul>
   </li>
   <li>test-4</li>
   </ul>
\$\endgroup\$
  • 1
    \$\begingroup\$ One suggestion, add var before your variables. \$\endgroup\$ – Rocket Hazmat Jul 9 '12 at 16:46
3
\$\begingroup\$

You want to make sure you always declare your variables before you assign. The reason for this is JavaScript will declare and assign in one statement but always on the global scope. This might overwrite important variables and cause you hours of debugging. The following has been cleaned up with some explanations.

// I personally prefer passing my ready callbacks directly to the jQuery function.
$(function() {
    // Declare a few constants for easier maintenance
    var delayTime = 200, slideDownTime = 400, slideUpSpeed = 'slow';

    // No need to declare or assign a variable here.
    $('#main > li.expand').click(function() { 
        // Lets declare this since we use it quite often
        var $this = $(this);
        // Modify the top element first 
        $this.removeClass('expand').addClass('minus');
        // No need to assign this result
        $this.find('ul.sub-level').delay(delayTime).slideDown(slideDownTime); 
        // Take advantage of the not selector
        $('#main > li > ul:not(.sub-level)').slideUp(slideUpSpeed);
        // Pass our declared variable instead and take advantage of toggleClass
        $('#main > li.minus').not($this).toggleClass('minus expand');
    });

    // Declare and assign. Maybe come up with a better name?
    var $li = $('.sub-level > li.expand');
    $li.click(function(){
        $(this).find('ul.sub-level2').delay(delayTime).slideToggle(slideDownTime);
        $li.toggleClass('minus expand');
    });      
});
\$\endgroup\$
  • 1
    \$\begingroup\$ $li.toggleClass('minus').toggleClass('expand'); could more simply be: $(this).toggleClass('minus expand'). \$\endgroup\$ – Rocket Hazmat Jul 9 '12 at 18:10
  • \$\begingroup\$ @Rocket - Nice catch, I didn't even notice it when I reviewed your answer. \$\endgroup\$ – ChaosPandion Jul 9 '12 at 18:37
  • \$\begingroup\$ In my answer, I abstracted that out to its own function. \$\endgroup\$ – Rocket Hazmat Jul 9 '12 at 18:39
6
\$\begingroup\$

Some suggestions:

  • Use var when declaring variables, unless you really want to make global variables (which you probably don't)
  • Combine var statements using ,

    var a = 1, b = 2, c = 3;
    
  • Combine classes in toggleClass

    $(this).toggleClass('minus expand')
    
  • Cache selectors that are used often (like $('#main'))
  • You don't need to use $li2 inside the click handler, you can just use $(this)
  • Turn repeated code into a function
$(function(){
    var $main = ('#main'),
    slide = function(ele, find){
        return $(ele).toggleClass('minus expand').find(find).delay(200).slideToggle(400);
    },
    $li = $('> li.expand', $main).click(function(){
        var $ul = slide(this, 'ul.sub-level');
        $('> li > ul', $main).not($ul).slideUp('slow');
        $('> li.minus', $main).not(this).toggleClass('minus expand')
    }),
    $li2 = $('.sub-level > li.expand').click(function(){
       slide(this, 'ul.sub-level2');
    });
});
\$\endgroup\$
  • \$\begingroup\$ You don't appear to need $li or $li2 at all. It might be useful though to extract and name some of the constants in there in case you need to discover/change them in the future. \$\endgroup\$ – Bill Barry Jul 9 '12 at 18:06
  • \$\begingroup\$ @BillBarry: I kept $li and $li2, because I figured this script was part of a bigger program, and that maybe they were used again elsewhere. \$\endgroup\$ – Rocket Hazmat Jul 9 '12 at 18:09
  • 1
    \$\begingroup\$ @Rocket - After reviewing your answer again I like the way you refactored the slide logic. \$\endgroup\$ – ChaosPandion Jul 9 '12 at 18:41
  • \$\begingroup\$ @ChaosPandion: Thanks. I prefer using functions instead of writing the same thing multiple times. \$\endgroup\$ – Rocket Hazmat Jul 9 '12 at 18:42
  • \$\begingroup\$ You don't need to use $li2 inside the click handler, you can just use $(this) - Why create a new jQuery object every time the element is clicked? \$\endgroup\$ – Joseph Silber Jul 10 '12 at 22:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.