Change CSS based on scroll position — Refactoring bad code

5 comments

1. All the previous answers will work just fine, because you have multiple ways to make this better just making some changes to your CSS selectors, but if you will do all this calculations in the scroll event, you should read this John Resign Post about how to deal with scroll event, specially this part:

   It’s a very, very, bad idea to attach handlers to the window scroll event. Depending upon the browser the scroll event can fire a lot and putting code in the scroll callback will slow down any attempts to scroll the page (not a good idea). Any performance degradation in the scroll handler(s) as a result will only compound the performance of scrolling overall. Instead it’s much better to use some form of a timer to check every X milliseconds OR to attach a scroll event and only run your code after a delay (or even after a given number of executions – and then a delay).
   – John Resign

   So, in your case I would go like this:

   HTML:

   <div class="menu">
       <a id="menu-item-22">Link 1</a>
       <a id="menu-item-23">Link 2</a>
       <a id="menu-item-24">Link 3</a>
       <a id="menu-item-25">Link 4</a>
       <a id="menu-item-26">Link 5</a>
   </div>
   

   Jquery:

   var didScroll = false;
   
   $(window).scroll(function() {
       didScroll = true;
   });
   
   setInterval(function() {
       if ( didScroll ) {
           didScroll = false;
           var $el;
   
           //Same that all the if else statements
           switch (true) {
               case (scroll >= 591 && scroll <= 1380):
                   $el = $("#menu-item-26 a");
                   break;
               case (scroll >= 1381 && scroll <= 2545):
                   $el = $("#menu-item-22 a");
                   break;
               case (scroll >= 2546 && scroll <= 2969):
                   $el = $("#menu-item-23 a");
                   break;
               case (scroll >= 2970):
                   $el = $("#menu-item-24 a");
                   break;
               default: //scroll<=590
                   $el = $("#menu-item-25 a");
           }
   
           //Removing blue class from all links
           $('.menu a').removeClass('blue');
   
           //Adding blue class to the matched element
           $el.addClass('blue');
       }
   }, 50);
   

   Log in to Reply

2. You shouldn’t be accessing a common group of elements using their ID’s –

   Example

   <div class="menu">
       <a id="menu-item-22 a">Link 1</a>
       <a id="menu-item-23 a">Link 2</a>
       <a id="menu-item-24 a">Link 3</a>
       <a id="menu-item-25 a">Link 4</a>
   </div>
   

   1 – Remove classes from all <a>‘s in .menu

   $(".menu a").removeClass("blue");
   

   2 – Display the one you want

   $("#menu-item-25 a").addClass("red");
   

   ---

   You should also consider caching the DOM selectors –

   var menuItem22 = $("#menu-item-22 a")

   and then access it like:

   $(menuItem22).addClass('red')

   This will prevent you from accessing the DOM each time you want to perform an op on the element. This is not absolutely necessary in your case but it’s a good trick to keep in mind

   Log in to Reply

3. How about this?

   http://pastebin.com/SrtE3yxS

   $(function() {
   
   var highlighted = null,         // Keep track of previously selected
       $menuItems  = $('#menu a'); // Grab all menu items once
   
   // Which items at which points
   var breakpoints = {
       '#menu-item-25': 590,
       '#menu-item-26': 1380,
       '#menu-item-22': 2545,
       '#menu-item-23': 2969,
       '#menu-item-24': 99999
   };
   
   $(window).scroll(function() {
   
           var scroll = $(window).scrollTop(),
           selected = '#menu-item-24';  // default
   
       // Loop through breakpoints to find the current one that should be selected
       for (id in breakpoints) {
           if (scroll <= breakpoints[id]) {
               selected = id;
               break;
           }
       }
   
       // If the right one is already highlighted, do nothing
       if (highlighted == id) {
           return;
       }
   
       // Remove all
       $menuItems.removeClass('blue');
       // Add it to the one you need
       $(id).addClass('blue');
   
   });
   

   });

   Log in to Reply

4. You could use a function to do the ever same thing..

    function menueHandle(ident, color, type){
       for(var i = 0; i < ident.length; i++){
           var handle = $("#menu-item-"+ident[i]+" a");
           if(type == 'remove'){
               handle.removeClass(color);
           }else{
               handle.addClass(color);
           }
       }
   }
   $(window).scroll(function() {
   var scroll = $(window).scrollTop();
   
   if (scroll <= 590) {
       menueHandle([25], 'blue', 'add');
       menueHandle([26,26,22,23, 24], 'blue', 'remove');
   }else if (scroll >= 591 && scroll <= 1380) {
       menueHandle([26], 'blue', 'add');
       menueHandle([25,26,22,23, 24], 'blue', 'remove');
   }else if (scroll >= 1381 && scroll <= 2545) {
       menueHandle([22], 'blue', 'add');
       menueHandle([25,26,22,23, 24], 'blue', 'remove');
   }else if (scroll >= 2546 && scroll <= 2969) {
       menueHandle([23], 'blue', 'add');
       menueHandle([25,26,22,23, 24], 'blue', 'remove');
   }else if (scroll >= 2970) {
       menueHandle([24], 'blue', 'add');
       menueHandle([25,26,22,23], 'blue', 'remove');
   } 
   

   });

   Log in to Reply

5. I’d like to add another approach (my personal favorite)

   • Use setTimeout() for scroll event handling

   • Use getBoundingClientRect() for detecting elements in view

   https://codepen.io/oriadam/pen/NLMqjN

   var vertical_threshold = 3; // pixels inside the element for it to be considered in view
   
   function element_in_view(e) {
       if (e.style.display == "none") {
           // when element is hidden, getBoundingClientRect() returns all 0
           return false;
       }
       var r = e.getBoundingClientRect();
       return (
           r.top >= 0 && // vertical check 1
           r.top <=
               (innerHeight || document.documentElement.clientHeight) -
                   Math.min(r.height, vertical_threshold) // vertical check 2
           /* CHECK HORIZONTAL SCROLL
                   && r.left >= 0 // horizontal check 1
                   && r.right <= (innerWidth || document.documentElement.clientWidth) // horizontal check 2
                   */
       );
   }
   
   function onScroll() {
       console.log("onScroll");
       $("li").each(function() {
           this.classList.toggle("inview", element_in_view(this));
       });
   }
   
   $(window).scroll(function() {
       clearTimeout(window.scrollTO);
       window.scrollTO = setTimeout(onScroll, 50);
   }).trigger('scroll');
   

   Log in to Reply