Grow your CSS skills. Land your dream job.

Care to give me some tips on how I could improve this code?

  • # March 27, 2012 at 10:25 pm

    Hi Joshua!

    You shouldn’t have to cycle through all of the styledSelects, rather it would be more efficient to just use a jQuery selector to find them (demo):

    // Show the unordered list when the styled div is clicked (also hides it if the div is clicked again)
    $styledSelect.click( function(e) {
    e.stopPropagation();
    $('div.styledSelect.active').each(function(){
    $(this)
    .removeClass('active')
    .next('ul.options').filter(':not(:animated)').slideUp(250);
    });
    $(this).toggleClass('active')
    .next('ul.options').filter(':not(:animated)').slideToggle(250);
    });

    Also I wouldn’t use === true because it isn’t necessary.

    # March 27, 2012 at 10:30 pm

    Oh and I also wanted to add a comment to @jamy_za’s suggestion in that the following isn’t good practice:

    // Insert an unordered list after the styled div
    styledSelect.after('
      ');

      I would use this format in a plugin just in case the user is using older versions of jQuery. The ability to include an object with parameters wasn’t added until jQuery 1.4, and some people are still stuck using older versions.

      # March 27, 2012 at 10:33 pm

      @Mottie I’m still learning when it comes to the wonderful world of jQuery, but isn’t using a for loop the better way to go? I’ve done some performance research and each() seems like a naughty tool.

      # March 27, 2012 at 10:44 pm

      @Mottie Fantastic advice, thank you for taking the time to help out! I have updated jsFiddle: http://jsfiddle.net/joshnh/eZzv7/

      # March 27, 2012 at 10:47 pm

      @TheDoc It would be if there were a lot of items to iterate through or if the each is called many times, say like on mouse move or window scroll. See this jspref example.

      But in this case, there are only two styledSelect divs; but even with 20 or 50, you probably won’t notice any performance issues.

      # March 27, 2012 at 10:57 pm

      Oh, also instead of using

      $list.filter(':not(:animated)').slideUp(250);

      to check for animation, you could just stop any ongoing animation, but include the clear que and jump to end flags (ref):

      $list.stop(true,true).slideUp(250);

      Updated demo.

      This works better for slower animation or rapid clickers. The dropdowns will now close instead of remaining open.

      # March 27, 2012 at 11:03 pm

      Thanks Rob, I’m not a fan of the way it handles clicks while the animation is running though. I agree that it would work well for slower animations. With the speed I am running the animation at there is no way a ‘rapid clicker’ could know what he wanted to click on before the animation was complete.

      Thanks for raising the performance issue too Gray, if I was to ever release this as a plugin, I would use a for loop.

      # March 28, 2012 at 2:03 am

      Oh and once more thing:

      $('div.styledSelect').eq(i).removeClass('active')

      Is the div in div.styleSelect necessary? Each thing you add there slows down the selecting process. So it would be faster to go $(‘.styleSelect’)

      # March 28, 2012 at 6:23 pm

      I was simply [over]qualifying the selector, thanks for the tip Jamy!

      # March 28, 2012 at 6:40 pm

      Actually @jamy_za, leave the “div” because it is faster. Also see tip #1.

      # March 28, 2012 at 9:34 pm

      Thanks for the clarification Rob!

      # March 29, 2012 at 1:01 am

      @Mottie I have been doing some testing/research and I have found that when simply using a class selector, over-qualifying it is slower: http://jsperf.com/over-qualified-jquery-selectors

      # March 29, 2012 at 1:03 am

      Thanks, @Mottie!

      # March 29, 2012 at 8:21 am

      @joshuanhibbert Hmm, that test does add an additional function, changing the background color, so I’m just not sure.

      And now that I think about it, the recommendations from tip #1 above, shows how to optimize use of the sizzle engine which only activates when you use multiple selectors… so yeah, it’s not relevant in this case.

      So at this point, I removed the background color from the jsperf and the class name only was faster, then I copied this post into the HTML to see if it’s faster with returning just one element and well the class name only was still faster… so yeah @jamy_za and you were right!

      I hereby retract my last statement ;)… you learn something new everyday :P

      # March 29, 2012 at 5:55 pm

      @Mottie You certainly do, and you have taught me quite a few things in this thread alone!

    Viewing 15 posts - 16 through 30 (of 30 total)

    You must be logged in to reply to this topic.

    *May or may not contain any actual "CSS" or "Tricks".