2
\$\begingroup\$

I have followed a tutorial that initially was for adding a 'sticky' nav. I adapted this slightly so it adds a background to the Nav when the page scroll reaches the top-third point of the hero area.

I then added a similar function that dims the hero area once the scroll gets to the midway point of the hero.

Both functions work in terms of displaying the expected results, but I am questioning the need for two almost identical functions and event listeners.

Could these be combined into one, complete function?

const hero = document.querySelector("#heroArea");
// Finds the top third of the element by adding the top of the element to the height of the element then divide by 3
const bottomOfNav = hero.offsetHeight / 3;
const middleHero = hero.offsetHeight / 2;

function fixNav() {
  // If the window position is greater or equal to the bottom of the nav
  if (window.scrollY >= bottomOfNav) {
    // Adds a class to the body tag
    document.body.classList.add("fixed-nav");
  } else {
    document.body.classList.remove("fixed-nav");
  }
}

window.addEventListener("scroll", fixNav);

function dimHero() {
  if (window.scrollY >= middleHero) {
    document.body.classList.add("dim-hero");
  } else {
    document.body.classList.remove("dim-hero");
  }
}

window.addEventListener("scroll", dimHero);
* {
  padding: 0; 
  margin: 0;
  font-family: sans-serif;
}

nav {
  position: fixed;
  width: 100%;
  left: 0;
  right: 0;
  color: #fff;
  padding: 15px;
  text-align: center;
  text-transform: uppercase;
  font-family: sans-serif;
  transition: .3s;
}

.fixed-nav nav {
  background: #333;
}

.hero {
  height: 100vh;
  background: black;
  opacity: 1;
  transition: .3s;
}

.dim-hero .hero {
  opacity: 0;
}

.text-block {
  height: 50vh;
  background: #555;
  color: #fff;
  font-size: 30px;
  text-align: center;
  padding: 40px;
}

.text-block-two {
  background: #333;
}
<body>
  <nav>Navigation</nav>
  <div id="heroArea" class="hero"></div>
  <div class="text-block">Lorem ipsum, dolor sit amet consectetur adipisicing elit. Eos ipsum et, omnis sit vero ab doloremque quia dolores mollitia. Doloremque maxime dolores quo eius ea. Ad, reiciendis minus. Dolorum, hic.</div>
  <div class="text-block text-block-two">Lorem ipsum, dolor sit amet consectetur adipisicing elit. Eos ipsum et, omnis sit vero ab doloremque quia dolores mollitia. Doloremque maxime dolores quo eius ea. Ad, reiciendis minus. Dolorum, hic.</div>
</body>

\$\endgroup\$
2
\$\begingroup\$

Bearing in mind that you already were able to abstract the functionality of toggling the class name into a separate function, I see other changes that can be made to clean up the code:

  1. Toggle Method: Use the toggle() method of classList to shorten the function changeClass()

    function changeClass(offset, clsName) {
        document.body.classList.toggle(clsName, window.scrollY >= offset);
    }
    
  2. DOM access method: Use getElementById() to select the element with Id heroArea instead of querySelector(). While it may likely never be noticeable on a sample page this small, it generally works faster. Check out this Sitepoint forum and this SO question and its answers (and related posts).

  3. Combined event handlers: The scroll event handlers can be combined to a single function that calls changeClass() twice.

    window.addEventListener("scroll", function() {
        changeClass(bottomOfNav, 'fixed-nav');
        changeClass(middleHero, 'dim-hero');
    });
    
  4. Arrow functions: Because features like const are used, others like arrow functions could be used for the functions- e.g.

    const changeClass = (offset, clsName) =>  document.body.classList.toggle(clsName, window.scrollY >= offset);
    

    Though some would argue that is too long for a single line, so brackets can be used:

    const changeClass = (offset, clsName) =>  {
        document.body.classList.toggle(clsName, window.scrollY >= offset);
    };
    
  5. CSS: combine selectors for common styles: I noticed there are two selectors that both have the same background style - those can be combined to a single ruleset:

    .fixed-nav nav,
    .text-block-two {
      background: #333;
    }
    

The changes have been applied to the sample code below.

const hero = document.getElementById("heroArea");
// Finds the top third of the element by adding the top of the element to the height of the element then divide by 3
const bottomOfNav = hero.offsetHeight / 3;
const middleHero = hero.offsetHeight / 2;
const changeClass = (offset, clsName) =>  document.body.classList.toggle(clsName, window.scrollY >= offset);
    
window.addEventListener("scroll", function() {
  changeClass(bottomOfNav, 'fixed-nav');
  changeClass(middleHero, 'dim-hero');
});
* {
  padding: 0; 
  margin: 0;
  font-family: sans-serif;
}

nav {
  position: fixed;
  width: 100%;
  left: 0;
  right: 0;
  color: #fff;
  padding: 15px;
  text-align: center;
  text-transform: uppercase;
  font-family: sans-serif;
  transition: .3s;
}

.fixed-nav nav,
.text-block-two {
  background: #333;
}

.hero {
  height: 100vh;
  background: black;
  opacity: 1;
  transition: .3s;
}

.dim-hero .hero {
  opacity: 0;
}

.text-block {
  height: 50vh;
  background: #555;
  color: #fff;
  font-size: 30px;
  text-align: center;
  padding: 40px;
}
<body>
  <nav>Navigation</nav>
  <div id="heroArea" class="hero"></div>
  <div class="text-block">Lorem ipsum, dolor sit amet consectetur adipisicing elit. Eos ipsum et, omnis sit vero ab doloremque quia dolores mollitia. Doloremque maxime dolores quo eius ea. Ad, reiciendis minus. Dolorum, hic.</div>
  <div class="text-block text-block-two">Lorem ipsum, dolor sit amet consectetur adipisicing elit. Eos ipsum et, omnis sit vero ab doloremque quia dolores mollitia. Doloremque maxime dolores quo eius ea. Ad, reiciendis minus. Dolorum, hic.</div>
</body>

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

In answer to my own question, I asked a colleague for their input and they came up with what I believe to be a much cleaner method.

My understanding of this code is that to make the code easier to maintain it is better to create one function that handles the adding and removing of the classes. This function takes two arguments, the offset number and the class name to be added/removed.

Each event listener runs an anonymous function that calls changeClass(), including the formula to get the offset and the class name to be added/removed.

(As my personal JS knowledge is quite weak at this time please feel free to update this explanation)

var hero = document.querySelector("#heroArea");

function changeClass(offset, clsName) {
  if (window.scrollY >= offset) {
    document.body.classList.add(clsName);
  } else {
    document.body.classList.remove(clsName);
  }
}

window.addEventListener("scroll", function() {
  // Two arguments passed to function
  changeClass(hero.offsetHeight / 3, "fixed-nav");
});

window.addEventListener("scroll", function() {
  changeClass(hero.offsetHeight / 2, "dim-hero");
});
| 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.