2
\$\begingroup\$

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!

\$\endgroup\$
  • 1
    \$\begingroup\$ Before I get started reviewing, have you linted your code? \$\endgroup\$ – kojiro Nov 10 '11 at 16:21
  • 1
    \$\begingroup\$ @kojiro No, actually its the first time I heard of this tool and thanks a lot for sharing!! After doing it: I get a couple of "Unnecessary semicolon" (I´d like to keep them though - in case they dont have any negative impact?). And "'smoothScrolling' is not defined." (thats because this function is outside my pasted example, but it does ofc exist in my whole code). Thanks again for sharing that tool! \$\endgroup\$ – r0skar Nov 10 '11 at 16:39
  • \$\begingroup\$ Oh and one more thing: "Line 3: var toggleOpen = $('*.[data-toggle="open"]'); Missing "use strict" statement." I am not sure what this means and how to solve it... \$\endgroup\$ – r0skar Nov 10 '11 at 16:41
  • \$\begingroup\$ "use strict" is a feature of newer versions of JS. It means to make sure you're not doing a bunch of stuff that people accidentally do that causes lots of headaches. To use it, basically, you put that string (just that string; no var, no whatever=, none of that. just "use strict";) as the first line of your function, and then fix all the errors you'll probably get. :) \$\endgroup\$ – cHao Nov 10 '11 at 16:48
  • \$\begingroup\$ Here's some information on strict mode. If you decide not to use it, you can turn it off in the linter's options. Get rid of the extra semicolons and read about the difference between function declarations and function expressions. As for smoothScrolling, you can just tell the linter that it's a global, and it won't complain anymore. \$\endgroup\$ – kojiro Nov 10 '11 at 16:51
3
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ Hi! Thanks for the improvements! I am gonna use closest() (edit: I ended up using parents() instead of closest()), since it makes more sense and I will remove the var nextContent. About the clicked variable: i thought that I maybe need to use $(this) more than once (not at the moment, but maybe when I expand the script with some more functionality) and I have read somewhere that when using it or any other selector more than once or twice, its a good idea to save it as a variable (not sure if thats true?). Thanks again! \$\endgroup\$ – r0skar Nov 10 '11 at 20:43
  • \$\begingroup\$ For some reason target.closest(toggleContent); doesnt work, but target.parents().find(toggleContent); does. I would like to use closest() though, because it would not return multiple elements (if there are more than one). I will try to find the problem and post the edit in my original question! \$\endgroup\$ – r0skar Nov 10 '11 at 20:52
  • 1
    \$\begingroup\$ Great! As for the $(this), it is indeed better to cache it if you intend to use it multiple times. However, you can just do that when (and if) you expand the script. \$\endgroup\$ – DADU Nov 10 '11 at 21:15
  • 1
    \$\begingroup\$ @Andrej I agree With DADU, I think the saying is: "Premature Optimisation is the root of all evil." \$\endgroup\$ – James Khoury Nov 10 '11 at 22:56
  • \$\begingroup\$ @James That's correct. A trap I've been fallen into too many times (I can sense if from far now). \$\endgroup\$ – DADU Nov 10 '11 at 23:33
2
\$\begingroup\$

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:

\$\endgroup\$
  • \$\begingroup\$ Thanks again for the time you spent to look into it! I will go with event.preventDefault \$\endgroup\$ – r0skar Nov 10 '11 at 17:44

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.