6
\$\begingroup\$

i am currently learning to do things with vanilla Javascript. No plugins, libraries or code snippets. I want to do things on my own, so that I understand 100% of what is happening.

I wrote a little script which checks, if a section of the page is in view.

Now, the script works as expected, but I was wondering, if I had accomplished the task in a good way. I am especially unsure about looping over the sections in a scroll event. I would love to get some feedback on this. Example page

Here is the code

(function () {
    // Store all panels in variable
    const panels = document.querySelectorAll(".panel");

    // Window on load event
    window.addEventListener("load", () => {
        let scrollTop = window.scrollY;
        let windowHeight = window.innerHeight;
        let documentHeight = document.body.offsetHeight;

        panels.forEach( panel => {
            if(scrollTop == panel.offsetTop ){
                panel.classList.add("is-visible");
            }
        });
    }); // end of load event

    // Window on scroll event
    window.addEventListener("scroll", () => {
        let scrollTop = window.scrollY;
        let windowHeight = window.innerHeight;
        let documentHeight = document.body.offsetHeight;

        panels.forEach( panel => {
            if(scrollTop >= panel.offsetTop - ( windowHeight / 2)){
                panel.classList.add("is-visible");
            } else {
                panel.classList.remove("is-visible");
            }
            if(scrollTop >= (panel.offsetTop + panel.clientHeight) - ( windowHeight / 2) ) {
                panel.classList.remove("is-visible");
            }
        });

    }); // end of scroll event
})();

APPENDIX

So I refactored the code as follows. I am still not sure about things like the IIFs.

(function(){


            const panels = document.querySelectorAll(".panel");
            let scrolling = false;


            (function windowOnLoad () {

                // Window on load event
                window.addEventListener("load", () => {
                    let scrollTop = window.scrollY;
                    let windowHeight = window.innerHeight;
                    let documentHeight = document.body.offsetHeight;

                    panels.forEach( panel => {
                        if(scrollTop == panel.offsetTop ){
                            panel.classList.add("is-visible");
                        }
                    });
                }); // end of load event
            })();


            (function windowOnSCroll(){
                window.addEventListener("scroll", () => {
                    scrolling = true;
                }); 
            })();


            setInterval( function() {
              if (scrolling) {
                scrolling = false;
                let scrollTop = window.scrollY;
                let windowHeight = window.innerHeight;
                let documentHeight = document.body.offsetHeight;

                panels.forEach( panel => {
                    if(scrollTop >= panel.offsetTop - ( windowHeight / 2)){
                        panel.classList.add("is-visible");
                    } else {
                        panel.classList.remove("is-visible");
                    }
                    if(scrollTop >= (panel.offsetTop + panel.clientHeight) - ( windowHeight / 2) ) {
                        panel.classList.remove("is-visible");
                    }
                });

              }
            }, 250 );


        })();
\$\endgroup\$
  • 1
    \$\begingroup\$ Hey, just a small tip. The scroll event fires a lot, and this may cause performance issues, you can resolve this by debouncing the event. \$\endgroup\$ – Vera Perrone Mar 19 at 13:54
  • \$\begingroup\$ Thanks for your comment. Actually I have never heart about debouncing events. I just did a quick research. So, what would be a good way to debounce? Depending on pixels or on time? \$\endgroup\$ – Dennis Fink Mar 19 at 16:07
  • 1
    \$\begingroup\$ I suggest something like this: google.com/amp/s/benmarshall.me/… \$\endgroup\$ – Vera Perrone Mar 19 at 16:17
2
\$\begingroup\$

I am especially unsure about looping over the sections in a scroll event.

There doesn't seem to be much of an alternative. One could select all elements with class name is-visible:

const visible = document.getElementsByClassName('is-visible');

Note that that collection is live, meaning it will be updated dynamically as the DOM changes, so there isn't any reason to re-query the DOM for those.

In the version that attempts to debounce the DOM updates, the IIFEs seem a bit excessive. Is the goal to limit scope of variables?


The function passed to panels.forEach() could be simplified:

panel => {
    if(scrollTop >= panel.offsetTop - ( windowHeight / 2)){
        panel.classList.add("is-visible");
    } else {
        panel.classList.remove("is-visible");
    }
    if(scrollTop >= (panel.offsetTop + panel.clientHeight) - ( windowHeight / 2) ) {
        panel.classList.remove("is-visible");
    }
}

This can be simplified by calling Element.classList.toggle() with two arguments.

panel => {
    let addClass = scrollTop >= panel.offsetTop - ( windowHeight / 2);

    if(scrollTop >= (panel.offsetTop + panel.clientHeight) - ( windowHeight / 2) ) {
        addClass = false;
    }
    panel.classList.toggle("is-visible", addClass);
}

const panels = document.querySelectorAll(".panel");

In most browsers it would generally be quicker to fetch the game squares using document.getElementsByClassName(), but that would return a live collection (refer to this SO answer for an explanation).

 const panels = [...document.getElementsByClassName("panel")];

The debounced version looks a little bit un-efficient because it uses an interval so that function gets executed more than it needs to.

There are also many arguments not to use setInterval and setTimeout - e.g. blindman67's SO Post Why is requestAnimationFrame better than setInterval or setTimeout

The functionality from the setInterval callback could be put into a function called checkVisibility. Then the scroll handler could be simplified to use requestAnimationFrame()

let timeout;
window.addEventListener("scroll", _ => {
    if (timeout) {
        window.cancelAnimationFrame(timeout);
    }
    timeout = window.requestAnimationFrame(checkVisibility);
}); 
\$\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.