Grow your CSS skills. Land your dream job.

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

  • # March 26, 2012 at 11:51 pm

    As I learn jQuery I choose a little project as often as I have time and attempt to make it on my own. I have spent the past little while creating a customised select element and would love to hear how it could be improved. Here is the fiddle: http://jsfiddle.net/joshnh/buQr9/

    Thanks in advance!

    # March 26, 2012 at 11:58 pm

    Hey Josh! Looking good, a few things:

    1. Click anywhere to close. I shouldn’t have to click on the select button again to close the choices.

    2. A little dropdown icon on the select might be nice, little UI enhancement.

    3. The sliding is a little bit slow to me.

    # March 27, 2012 at 12:05 am

    Thanks Gray, all great points! Would you mind checking it now and telling me if that’s the best way to hide the drop down when clicking away from it?

    # March 27, 2012 at 12:34 am

    That looks pretty good, though I’m no jQuery expert!

    The other thing you’ll want to do is make sure the user can’t queue up a bunch of animations.

    # March 27, 2012 at 12:40 am

    Hmm, another great point, but I’m not entirely sure on how to implement it. Simply using .stop() before sliding the list up or down leads to a strange situation where it won’t show it all if you click on the div a few times in quick succession.

    EDIT: Chris has a solution on this very site: http://css-tricks.com/full-jquery-animations/

    # March 27, 2012 at 1:30 am

    So I discovered a major flaw: as soon as I had more than one select element, the whole thing broke. It has now been fixed and the latest version is here: http://jsfiddle.net/joshnh/fyqdS/

    # March 27, 2012 at 3:05 am

    I was going to mention multiple select elements, but didn’t want to push! haha

    Here’s another one for you: if you have one dropdown open, when you click on a new one it should close the previous one.

    # March 27, 2012 at 3:37 am

    Right, I’ll have a think and get that implemented. Thanks Gray :)

    # March 27, 2012 at 3:45 am

    I can give you a hint in plain English!

    When you click any select button, you should loop through all of them and see if any of them have an ‘active’ state. If they do, close it!

    # March 27, 2012 at 4:52 am

    Great, thanks!

    # March 27, 2012 at 7:04 am

    Instead of looping, would jQuery’s .find() not do it? maybe not though

    For the record, my favourite animal is giraffe

    # March 27, 2012 at 9:21 am

    is .each() the way t do it?

    # March 27, 2012 at 9:38 am

    It’s pretty good practice to add a $ to the beginning of jQuery object variables. So:

    // Cache the select form element/s
    var $select = $('select'),
    selectHeight = $select.height(),
    numberOfOptions = select.children('option').length;

    You don’t have to do that though – It just seems to be common practice now and I do it
    I think it’s best to keep CSS out of javascript completely. Rather add a class.

    // Hides the select element
    $select.addClass('hidden');

    This isn’t really good practice:

    // Insert an unordered list after the styled div
    styledSelect.after('
      ');
      var ul = $('ul.options');

      It’s better to do something like:

      var $listOptions = $('
        ', {
        'class' : 'options'
        }).insertAfter($styledSelect);

        It’s not really scalable or neat to just add strings of stuff so it’s better to create and cache jQuery objects to a variable that way. makes your code much neater too.

        Your way:

        // Insert a list item into the unordered list for each select option
        for(i = 0; i < numberOfOptions; i++) {
        ul.append('
      • ' + select.children('option').eq(i).text() + '
      • ');
        }

        Better way:

        for(var i = 0; i < numberOfOptions; i++) {
        $('
      • ', {
        text: $select.children('option').eq(i).text()
        }).appendTo( $listOptions);
        }

        remember to add var i otherwise you are setting the global i value which isn’t what you want to do there.

        @tobeeornot I only use the jQuery .each() function if I have to. It’s easy to do a quick javascript loop which is much lighter on the browser.

        For the record, I’m not a cat person.

        # March 27, 2012 at 9:41 am

        @jamy_za Thanks for that. I’m still coming to grips with jquery so it good to know these things! That code looks nice and neat too :)

        I like wombats!

        # March 27, 2012 at 6:41 pm

        @jamy_za Wow! Thank you so much mate, that is incredibly helpful! I have implemented your changes (after making sure I understand them), and the ones recommend by @TheDoc, and this is where it is currently at: http://jsfiddle.net/joshnh/eZzv7/

        Is this the best way to close any other select that may be open when clicking on a new one?

        for(var i = 0; i < numberOfSelects; i++) {
        if($('div.styledSelect').eq(i).hasClass('active') === true) {
        $('div.styledSelect').eq(i).removeClass('active')
        .next('ul.options').filter(':not(:animated)').slideUp(250);
        }
        }

        Also, is it best practise to explicitly state ‘=== true’, or can I leave that off?

        Oh, and if you look towards the bottom of the code, you will see that I am having difficulty assigning the the clicked list item to the value of the select. Any suggestions?

        EDIT: I think I have solved that last issue by assigning $(this) to the variable $this.

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

      You must be logged in to reply to this topic.

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