jQuery script to toggle an element and handle a Close button

Question

First of all: I am a total javascript beginner and therefore I am asking you to rate my script and tell me whether its okay or just a big mess. Note: it does work, but I guess it could be improved.

My main goal was to create a script, than can be used multiple times and does not depend on any class or id name (thats why I am using the data attribute).

$(function(){
    var toggleOpen      = $('*.[data-toggle="open"]');
    var toggleContent   = $('*.[data-toggle="content"]');
    var toggleClose     = $('*.[data-toggle="close"]');
    var toggleSpeed     = 500;
    //Set height of toggle content to avoid animation jumping
    toggleContent.css('height', toggleContent.height() + 'px');
    //Find content to toggle
    function findNextContent(target){
        var nextContent = target.parent().parent().parent().find(toggleContent);
        return nextContent;
    }
    //Toggle content
    function slideToggle(target){
        target.stop(true,true).slideToggle(toggleSpeed);
    }
    //CLose toggled content
    function closeToggle(target){
        target.slideUp(toggleSpeed);
    }
    //On Open Click
    toggleOpen.click(function(){
        var clicked     = $(this);
        var nextContent = findNextContent(clicked);
        //Check if hidden to either scroll to bottom or not
        if(nextContent.is(':hidden')){
            slideToggle(nextContent);
            smoothScrolling(toggleClose);
        }else{
            slideToggle(nextContent);
        }
        return false;
    });
    //On Close click
    toggleClose.click(function(){
        var clicked     = $(this);
        var nextContent = findNextContent(clicked);
        closeToggle(nextContent);
        return false;
    });
});

What it does: It toggles an element and it also takes care of a seperate close button.

I am glad for any feedback - be it positive or negative!

Answer 1

function findNextContent(target){
    var nextContent = target.parent().parent().parent().find(toggleContent);
    return nextContent;
}

You could possibly use the jQuery closest function here. Or you could remove the variable and just do return target.parent().parent().parent().find(toggleContent);

toggleClose.click(function(){
    var clicked     = $(this);
    var nextContent = findNextContent(clicked);
    closeToggle(nextContent);
    return false;
});

There is no need for the clicked variable. You can just write findNextContent($(this));

You might also want to look into the aria-expanded attribute.

Answer 2

OK, this code looks fine to me. The only thing I can suggest is that you might prefer to use event.preventDefault instead of return false in your event handlers, because in jQuery, return false in an event handler causes both event.preventDefault() and event.stopPropagation(), which you may not want.

References:

• https://stackoverflow.com/questions/1357118/javascript-event-preventdefault-vs-return-false
• http://api.jquery.com/event.preventDefault/