1
\$\begingroup\$

I have the following code, though I'm not sure it is efficient as it could be.

 $(window).scroll(function() {
    var scrollT = $(document).scrollTop();
    if (scrollT >= 180) {
        $("#primary-nav-wrapper").addClass("scroll");
        $("#primary-nav-wrapper li.front").addClass("active");


        $("#primary-nav-wrapper .search-wrapper").removeClass("active");
        $("#primary-nav-wrapper .search-field").blur();
    }
    else {
        $("#primary-nav-wrapper").removeClass("scroll");
        $("#primary-nav-wrapper .search-wrapper, #primary-nav-wrapper li.front").removeClass("active");

        $("#primary-nav-wrapper .search-field").blur();
    }
    if (scrollT >= 400) {$("a#to-top-link").addClass("active");}
    else {$("a#to-top-link").removeClass("active");}
});

Basically, what I do here is checking two if-clauses every time I scroll, but is it more resource-friendly to only check every few milliseconds? If so, how is this done? Or is it a better idea to re-write the if-else structure, e.g. if.. else if... else if... else?

\$\endgroup\$
2
  • \$\begingroup\$ What does setting the active class accomplish? Could you make a live Stack Snippet example including some HTML and CSS? \$\endgroup\$ Commented Jan 27, 2015 at 20:36
  • \$\begingroup\$ @200_success The classes trigger CSS3 animations, which I prefer over JS enabled ones. I don't think a live example would be helpful because the code works perfectly, but I was simply wondering if it could be optimised a bit. \$\endgroup\$ Commented Jan 28, 2015 at 10:30

1 Answer 1

1
\$\begingroup\$

I don't know if you mean general performance, but one thing I've heard helps a lot is to put all of your different selectors in variables outside of the function as it is apparently pretty expensive to make the selector calls again and again. I mean something like this (and I do mean "something like this" because I haven't tested even the basics, but the general idea is described here: http://www.artzstudio.com/2009/04/jquery-performance-rules/#cache-jquery-objects)

var scroll = {
     primary  : $("#primary-nav-wrapper"),
     front    : $("#primary-nav-wrapper li.front"),
     search_w : $("#primary-nav-wrapper .search-wrapper"),
     search_f : $("#primary-nav-wrapper .search-field"),
     to_top   : $("a#to-top-link")
}
$(window).scroll(function() {
    var scrollT = $(document).scrollTop();
    if (scrollT >= 180) {
        scroll.primary.addClass("scroll");
        scroll.front.addClass("active");


        scroll.search_w.removeClass("active");
        scroll.search_f.blur();
    }
    else {
        scroll.primary.removeClass("scroll");
        scroll.front.removeClass("active");

        scroll.search_f.blur();
    }
    if (scrollT >= 400) {scroll.to_top.addClass("active");}
    else {scroll.to_top.removeClass("active");}
});
\$\endgroup\$
2
  • \$\begingroup\$ Would it possibly be an even further optimisation when saying for instance front: primary.find("li.front")? \$\endgroup\$ Commented Jan 28, 2015 at 10:29
  • \$\begingroup\$ Sure. But a very minor one as you're only doing it once. Mostly in your case, you want to keep the dom traversal out of the "scroll" part because that is firing a lot (on every user scroll). For the set up, which runs only once per page load, I'd go with whatever is clearer to you. \$\endgroup\$
    – dmgig
    Commented Jan 28, 2015 at 15:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.