4
\$\begingroup\$

I have a scrollHandler function which contains all of my scroll event-listener logic.

I've used multiple IIFE's inside to separate the logic:

import skills from './json/skills.json';

let capToggle = false;
let triggerCalculateHeight = false;
let scrollEnd = false;

function headingFadeIn(heading) {
    if (!heading) return;
    const parent = heading.parentNode.parentNode;
    const triggerHeight = parent.offsetTop - window.innerHeight / 1.5;

    if (window.scrollY > triggerHeight) {
        heading.style.opacity = '1';
    } else heading.style.opacity = '0';
}

export default function scrollHandler() {
    // Headings
    (() => {
        headingFadeIn(document.getElementById('portfolio-heading'));
        headingFadeIn(document.getElementById('skills-heading'));
        headingFadeIn(document.getElementById('bio-heading'));
        headingFadeIn(document.getElementById('contact-heading'));
    })();
    // Skills
    (() => {
        const bars = document.querySelectorAll('.skill-bar');
        if (!bars) return;

        bars.forEach((i, index) => {
            const bottom = i.getBoundingClientRect().bottom;

            if (window.scrollY > bottom && !i.dataset.heightCalculated) {
                i.style.transform = `scale3d(1, ${skills[index].height / 10}, 1)`;
                i.setAttribute('data-height-calculated', true);

                capToggle = true;
                triggerCalculateHeight = true;
            }
        });
    })();
    // Reviews
    (() => {
        const el = document.getElementById('reviews-inner');
        if (!el) return;

        const mainBackground = document.getElementById('ul-bg');
        const backgrounds = document.querySelectorAll('.blockquote-bg');
        const texts = document.querySelectorAll('.reviews-toggle');
        const bound = el.getBoundingClientRect().top * 10;

        if (window.scrollY > bound) {
            mainBackground.style.opacity = '1';
            mainBackground.style.transform = 'scale(1) rotate(45deg)';
            backgrounds.forEach(i => (i.style.opacity = '1'));
            backgrounds[0].style.transform = 'translate(-4%, -9%) skew(45deg)';
            backgrounds[1].style.transform = 'translate(-6%, -2%) skew(45deg)';
            backgrounds[2].style.transform = 'translate(-5%, 1%) skew(45deg)';
            texts.forEach(i => (i.style.opacity = '1'));
        }
    })();
    // Bio
    (() => {
        const container = document.getElementById('bio-container');
        const tabSelected = document.getElementById('tab-selected');
        const tabBackground = document.getElementById('tab-background');
        const height = document.getElementById('bio').offsetTop;
        if (!tabBackground || !container || !tabSelected || !height) return;

        if (window.scrollY > height / 1.1) {
            // Background tab styling
            tabBackground.style.left = `${tabSelected.offsetLeft}px`;
            tabBackground.style.maxWidth = `${tabSelected.offsetWidth + 20}px`;

            // Container transition
            container.style.opacity = '1';
            container.style.transform = 'skew(-25deg)';
            container.childNodes[0].style.transform = 'skew(25deg)';
        }
    })();
    // Contact
    (() => {
        const groups = document.querySelectorAll('.form-group');
        if (!groups) return;

        groups.forEach((i, index) => {
            i.childNodes[1].blur();

            if (window.scrollY > (i.getBoundingClientRect().top * 10) / 1.5) {
                i.style.opacity = '1';

                if (index >= groups.length - 1) scrollEnd = true;
            }
        });
    })();

    return { capToggle, triggerCalculateHeight, scrollEnd };
}

Is this considered good practice? Are there any pitfalls that could come from this?

\$\endgroup\$
0
3
\$\begingroup\$

Modules You're already using export syntax, so you're currently in a module. Rather than defining multiple IIFEs, you might consider using separate sub-modules:

// fadeInHeadings.js

// Renaming from headingFadeIn to fadeInHeading for readability
function fadeInHeading(heading) {
    if (!heading) return;
    const parent = heading.parentNode.parentNode;
    const triggerHeight = parent.offsetTop - window.innerHeight / 1.5;

    if (window.scrollY > triggerHeight) {
        heading.style.opacity = '1';
    } else heading.style.opacity = '0';
}

export const fadeInHeadings = () => {
  const headingIds = [
    'portfolio-heading',
    'skills-heading',
    'bio-heading',
    'contact-heading',
  ];
  for (const id of headingIds) {
    fadeInHeading(document.getElementById(id));
  }
};
// scrollHandler.js
import { fadeInHeadings } from './fadeInHeadings';
import { transformBars } from './transformBars';
import { transformReviews } from './transformReviews';

export default function scrollHandler() {
  fadeInHeadings();

  const { capToggled, newTriggerCalculateHeight } = transformBars();
  capToggle ||= capToggled;
  triggerCalculateHeight ||= newTriggerCalculateHeight;

  transformReviews();

  // ...

This way, when, for example, a review isn't scaling properly, you can navigate directly to transformReviews to debug and fix the problem, rather than wander around in a significantly larger file looking for where reviews are referenced.

Variable names and iteration Your two forEaches are a bit confusing:

bars.forEach((i, index) => {

When i is used as a variable name, it's almost always used to reference the index of a collection. To have i not only not refer to an index, but to also have an index variable, is confusing. Instead, consider:

bars.forEach((bar, index) => {

Same sort of thing in Contact:

const groups = document.querySelectorAll('.form-group');
if (!groups) return;
groups.forEach((i, index) => {

i should probably be renamed to formGroup or something similar. Also, querySelectorAll will return an array-like NodeList. Even if it's empty, it'll never be falsey, so you can remove the if (!groups) return; line.

children You have

formGroup.childNodes[1].blur();

childNodes returns a collection that includes text nodes. Text nodes can be empty, and including them in a collection can make logic more tedious than it needs to be. Since you want to select an element with that, better to use .children (which only returns element children nodes), so you can do

formGroup.children[1].blur();
// or
formGroup.children[0].blur();
// or, be more specific and use a CSS selector
formGroup.querySelector(<some-selector>).blur();

Persistent variables? The persistent module variables seem a bit odd. If they really need to be in persistent state, it might make sense to expose a function that returns their current values. If they aren't meant to be persistent, initialize them to false inside scrollHandler instead - or, inside the sub-module which reassigns its value, which is returned to scrollHandler and then returned by scrollHandler. For example, it'd be great if you could do this:

// showFormGroups.js
export const showFormGroups = () => {
    const groups = document.querySelectorAll('.form-group');
    let scrollEnd = false;
    groups.forEach((formGroup, index) => {
        formGroup.children[1].blur();
        if (window.scrollY > (formGroup.getBoundingClientRect().top * 10) / 1.5) {
            formGroup.style.opacity = '1';
            if (index >= groups.length - 1) {
                scrollEnd = true;
            }
        }
    });
    return scrollEnd;
};
// scrollHandler.js
export default function scrollHandler() {
    // ...
    const scrollEnd = showFormGroups();
    // ...
    return { capToggle, triggerCalculateHeight, scrollEnd };

Styles into CSS You have

if (window.scrollY > bound) {
    mainBackground.style.opacity = '1';
    mainBackground.style.transform = 'scale(1) rotate(45deg)';
    backgrounds.forEach(i => (i.style.opacity = '1'));
    backgrounds[0].style.transform = 'translate(-4%, -9%) skew(45deg)';
    backgrounds[1].style.transform = 'translate(-6%, -2%) skew(45deg)';
    backgrounds[2].style.transform = 'translate(-5%, 1%) skew(45deg)';
    texts.forEach(i => (i.style.opacity = '1'));
}

To apply this logic, I think it'd make more sense to put the CSS rules in your CSS file, and then just toggle a class. Something like:

// SCSS for brevity
body.showReviews {
  #ul-bg {
    opacity: 1;
    transform: scale(1) rotate(45deg);
  }
  .blockquote-bg {
    opacity: 1;
  }
  .blockquote-bg:nth-of-type(1) {
    transform: translate(-4%, -9%) skew(45deg);
  }
  .blockquote-bg:nth-of-type(2) {
    transform: translate(-6%, -2%) skew(45deg);
  }
  .blockquote-bg:nth-of-type(3) {
    transform: translate(-5%, 1%) skew(45deg);
  }
  .reviews-toggle {
    opacity: 1;
  }
}
// transformReviews.js
export const transformReviews = () => {
  const el = document.getElementById('reviews-inner');
  if (!el) return;
  const bound = el.getBoundingClientRect().top * 10;
  document.body.classList.toggle('showReviews', window.scrollY > bound);
};

Framework On a larger scale, all of this careful element selecting, iterating, examining, and manual manipulation of the DOM seems a bit tedious. For a professional project that isn't tiny, I would prefer to look into a framework like React to tie element state directly to their respective DOM elements.

\$\endgroup\$
2
  • \$\begingroup\$ This is very in-depth! Really appreciate it. When I'm not bleary-eyed I'll give it a full look over \$\endgroup\$ Sep 26 '20 at 20:15
  • \$\begingroup\$ Never mind, I couldn't help myself. This is brilliant, lot's of helpful tips. I'll definitely put some of these into practice, thanks again \$\endgroup\$ Sep 26 '20 at 20:20

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.