2
\$\begingroup\$

I have a scroll event and add class and remove classes for different elements. As you can see the code is a bit big. Is this method ok or is there a more efficient one?

I mean, you can see that there are no child elements. I could use .siblings() for some but, as you can see, I also serve unique classes.

Any idea? I know it looks bad but I think it is the only way.

 $(document).ready(function () {
        $(window).scroll(function () {
            var scroll = $(window).scrollTop();

            if (scroll >= 50) {
                $("nav ul li").addClass("list-mini");
                $("nav").addClass("nav-mini");
                $("nav ul li a").addClass("nav-recolor");
                $("nav img").removeClass("zoomOutLeft");
                $("nav img").addClass("zoomInUp");
                $("nav img").addClass("logo-mini-active");
                $("nav ul").addClass("margin-transition");
                $("ul").addClass("ul-active");
                $("nav ul li a span").addClass("text-removed");
                $("nav ul li a").addClass("nav-font");
                $("nav ul li a span").addClass("transition-02s");
                $(".fa-shopping-cart").addClass("fa-shopping-cart-mini");
                $(".fa-globe").addClass("fa-globe-mini");
                $(".fa-info").addClass("fa-info-mini");
                $(".fa-circle-o-notch").addClass("fa-circle-o-notch-mini");
                $(".fa-envelope-o").addClass("fa-envelope-o-mini");
            } else {
                $("nav ul").removeClass("margin-transition");
                $("nav ul li").removeClass("list-mini");
                $("nav").removeClass("nav-mini");
                $("nav ul li a").removeClass("nav-recolor");
                $("nav img").removeClass("zoomInUp");
                $("nav img").addClass(" zoomOutLeft");
                $("ul").removeClass("ul-active");
                $("nav ul li a span").removeClass("text-removed");
                $("nav ul li a").removeClass("nav-font");
                $("nav ul li a span").removeClass("transition-02s")
                $(".fa-shopping-cart").removeClass("fa-shopping-cart-mini");
                $(".fa-globe").removeClass("fa-globe-mini");
                $(".fa-info").removeClass("fa-info-mini");
                $(".fa-circle-o-notch").removeClass("fa-circle-o-notch-mini");
                $(".fa-envelope-o").removeClass("fa-envelope-o-mini");
            }
        });
    });
Improve this question
\$\endgroup\$
  • \$\begingroup\$ Hi. Welcome to Code Review! I think that we would need more context to give meaningful comments on this code. It's possible that you might cut this down with the right data structure, but the bigger question is if you could do something other than changing fifteen classes with every change. That seems to be an awful lot of changes for a single event, and there is no way for us to know what any particular change does. \$\endgroup\$ – Brythan Nov 9 '14 at 6:20
  • \$\begingroup\$ can you create a fiddle ? need to see your html structure one solution would be you can add a class to parent and style its children with that class \$\endgroup\$ – Vitorino fernandes Nov 9 '14 at 9:04
3
\$\begingroup\$

You shouldn't be doing so much manipulation of the classes. Instead, you could just add or remove a scrolled-down class to the <body> element. Then specify the rest of the changes using pure CSS. (For example, you would have a body.scrolled-down nav { … } rule.)

Improve this answer
\$\endgroup\$
0
\$\begingroup\$

One thing you could do is create two functions that adds/removes a class using two parameters:

function foo(element, className) {
    $('' + element + '').addClass('' + className + '');
}

function foo(element, className) {
    $('' + element + '').removeClass('' + className + '');
}

And then just call the function so you don't have to repeat yourself every time.

OR another way would be to put all the elements you want to manipulate and the classes in two arrays, and then just call the functions using a loop and the index in the array.

var myElements = ['nav img', '.fa-shopping-cart', ...];
var myClasses = ['zoomIn', ...];

for (i = 0; i < myElements.length; i++) {

    foo(myElements[i], myClasses[i]);
}

This might need a bit of editing but you get the idea.

However I'm not sure if any of these are actually better ways of doing what you want, but it's other ways of doing it. It might make everything more manageable if the code gets even bigger. Hope this helped.

Improve this answer
\$\endgroup\$

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.