2
\$\begingroup\$

I'm learning JS/jQuery at the moment and have built the following standalone script but I feel that it can probably be improved in some ways?

For example - is there a better way of switching the selected text on the .dropdown-menu li a click event rather than needing to replace the entire HTML?

Constructive advice welcome.

EDIT: Added HTML to help with context:

<div class="dropdowncontain">
    <button type="button" class="button defaultbutton">Default Value <span class="caret invert"></span></button>
        <ul class="dropdown-menu">
            <li><a href="#">Katie</a></li>
            <li><a href="#">Richard</a></li>
            <li><a href="#">Matthew</a></li>
            <li><a href="#">Sophie</a></li>
            <li class="dropdown-divider"></li>
            <li><a href="#">Default Value</a></li>
        </ul>
</div>

<div class="dropdowncontain">
    <button type="button" class="button primarybutton">Click Me <span class="caret"></span></button>
        <ul class="dropdown-menu">
            <li><a href="#">Katie</a></li>
            <li><a href="#">Richard</a></li>
            <li><a href="#">Matthew</a></li>
            <li><a href="#">Sophie</a></li>
        </ul>
</div>

So currently the script checks if the user is clicking in a dropdown menu that needs an inverted caret.

$(function() {
// Handles initial dropdown click event, if any dropdown menu is visible close it
$('.dropdowncontain').click(function(e){
    if (!$('.dropdown-menu', this).is(':visible')) {
        $('.dropdown-menu').hide();
    }
    $('.dropdown-menu', this).toggle();
    // Stop the bubbling of the event to the document level
    e.stopPropagation(); 
});

// Handles hiding the dropdown menu's when the user clicks elsewhere in the document
$(document).click(function() {
    $('.dropdown-menu').hide();
});

// Handles updating the text in the button and highlighting the selected option
$('.dropdown-menu li a').click(function(e){
    var parentBtn = $(this).parents('.dropdowncontain').find('.defaultbutton');
    var that = $(this).parents('.dropdowncontain').find('.dropdown-menu li a');
    var selectedText = $(this).text();

        if (that.hasClass('highlightdropdown')){
            that.removeClass('highlightdropdown');
        }

    $(this).addClass('highlightdropdown');  

    // If the containing parent div has a class with 'defaultbutton' add the invert class to the caret
    if (parentBtn.hasClass('defaultbutton')) {
        $(this).parents('.dropdowncontain').find('.button').html(selectedText+' <span class="caret invert"></span>');
    }
    else {
        $(this).parents('.dropdowncontain').find('.button').html(selectedText+' <span class="caret"></span>');
    }
    // Prevent the page jumping to the top when a link with no source is selected
    e.preventDefault();
});
});
\$\endgroup\$
2
\$\begingroup\$

There is no reason to swap the text, find the span in the button and toggle the class

var isDefault = parentBtn.hasClass('defaultbutton')
$(this).parents('.dropdowncontain').find('.button .caret').toggleClass("invert", isDefault);

The context selector is a bad idea

$('.dropdown-menu', this)

It is slow. Use find()

$(this).find('.dropdown-menu')

Another thing, $(this) is used over and over again. That is bad practice. It creates a new jQuery object each time. Store it in a variable and reuse that variable.

var myElement = $(this);
myElement.find('.dropdown-menu')...
\$\endgroup\$
  • \$\begingroup\$ Thanks for the advice I didn't realise .find() is more perfomant! I am swapping the text in order to update the button's text with what the user selected (increased UX). Also there is a missing semicolon in isDefault var - thought I'd just let you know! \$\endgroup\$ – Rich Jan 3 '14 at 17:21

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.