vanilla js eventListener on navigation

Question

I have built a "mega menu" that is working, but I would appreciate some advice on how my javascript can be improved.

Step 1 is looping through the top level menu items and applying the correct megamenu class names where necessary:

const megamenu = document.getElementById('megamenu');
let megamenuChildren = megamenu.children;
let megamenuChildrenArray = Array.from(megamenuChildren);

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

    let sub = megamenuChildrenArray[i].children;

    // if the nav li contains x1 children it is a regular link
    // if the nav li contains x2 children it is a megamenu item and we add the classes

    if (sub.length === 2) {

        let subArray = Array.from(sub);
        subArray[0].classList.add('megamenu-trigger-' + i);
        subArray[1].classList.add('megamenu-sub-' + i);

    }
}

Step 2 Earlier in my development I learned that we shouldn't add eventListener inside a loop! So instead I am adding an eventListener to the document to listen for all clicks.

There are only ever a maximum of x5 li's in the top level menu, so I am checking each one for a click and swapping the class 'dn' (display: none;) for 'db' (display: block;). I am also adding the class 'megamenu-sub-triggered':

document.addEventListener('click', function(event) {

    if (event.target.matches('.megamenu-trigger-0')) {
        console.log('trigger 0 clicked!');
        event.preventDefault();
        let subZero = document.getElementsByClassName('megamenu-sub-0')[0];
        subZero.classList.remove('dn');
        subZero.classList.add('db', 'megamenu-sub-triggered');
    }

    if (event.target.matches('.megamenu-trigger-1')) {
        console.log('trigger 1 clicked!');
        event.preventDefault();
        let subOne = document.getElementsByClassName('megamenu-sub-1')[0];
        subOne.classList.remove('dn');
        subOne.classList.add('db', 'megamenu-sub-triggered');
    }

    if (event.target.matches('.megamenu-trigger-2')) {
        console.log('trigger 2 clicked!');
        event.preventDefault();
        let subTwo = document.getElementsByClassName('megamenu-sub-2')[0];
        subTwo.classList.remove('dn');
        subTwo.classList.add('db', 'megamenu-sub-triggered');
    }

    if (event.target.matches('.megamenu-trigger-3')) {
        console.log('trigger 3 clicked!');
        event.preventDefault();
        let subThree = document.getElementsByClassName('megamenu-sub-3')[0];
        subThree.classList.remove('dn');
        subThree.classList.add('db', 'megamenu-sub-triggered');
    }

    if (event.target.matches('.megamenu-trigger-4')) {
        console.log('trigger 4 clicked!');
        event.preventDefault();
        let subFour = document.getElementsByClassName('megamenu-sub-4')[0];
        subFour.classList.remove('dn');
        subFour.classList.add('db', 'megamenu-sub-triggered');
    }
}

Step 3 Finally, inside the same document.addEventListener above, I am listening for clicks outside of the '.megamenu-sub-triggered' element, and checking each possible megamenu to see (a) if it exists, and (b) if it contains the triggered class.

If conditions (a) and (b) are met, I am putting the classes back to their original state:

if (document.getElementsByClassName('megamenu-sub-triggered')) {
    if (!event.target.closest('.megamenu-sub-triggered')) {

        var subZero = document.getElementsByClassName('megamenu-sub-0')[0];
        if (subZero && subZero.matches('.megamenu-sub-0.megamenu-sub-triggered')) {
            subZero.classList.remove('db', 'megamenu-sub-triggered');
            subZero.classList.add('dn');
        }

        var subOne = document.getElementsByClassName('megamenu-sub-1')[0];
        if (subOne && subOne.matches('.megamenu-sub-1.megamenu-sub-triggered')) {
            subOne.classList.remove('db', 'megamenu-sub-triggered');
            subOne.classList.add('dn');
        }

        var subTwo = document.getElementsByClassName('megamenu-sub-2')[0];
        if (subTwo && subTwo.matches('.megamenu-sub-2.megamenu-sub-triggered')) {
            subTwo.classList.remove('db', 'megamenu-sub-triggered');
            subTwo.classList.add('dn');
        }

        var subThree = document.getElementsByClassName('megamenu-sub-3')[0];
        if (subThree && subThree.matches('.megamenu-sub-3.megamenu-sub-triggered')) {
            subThree.classList.remove('db', 'megamenu-sub-triggered');
            subThree.classList.add('dn');
        }

        var subFour = document.getElementsByClassName('megamenu-sub-4')[0];
        if (subFour && subFour.matches('.megamenu-sub-4.megamenu-sub-triggered')) {
            subFour.classList.remove('db', 'megamenu-sub-triggered');
            subFour.classList.add('dn');
        }
    }
}

Any help improving this code, and my own javascript knowledge, is very much appreciated!

Many thanks

Answer 1

Step 1

Concise iteration and selectors

.children returns an array-like collection of elements. While the collection isn't actually an array, it still has elements at numeric indicies and has a .length. So, there's no need to convert it into an actual array first if you're iterating over it with a for (let i = 0; i < collection.length; ...) loop. So, with:

let megamenuChildren = megamenu.children;
let megamenuChildrenArray = Array.from(megamenuChildren);
for (let i = 0; i < megamenuChildrenArray.length; i++) {

you can instead use:

let megamenuChildren = megamenu.children;
for (let i = 0; i < megamenuChildren.length; i++) {

But there's a better option. Since you don't really care about the index being iterated over, you just care about the elements in the collection, how about iterating through only the elements, instead of the indicies? Use for..of instead:

for (const child of megamenuChildren) {

But there's a better option. Rather than selecting the #megamenu element, and then using .children to get to its children, then iterating over the children, you can use a selector string instead to get to the children immediately:

for (const child of document.querySelectorAll('#megamenu > *')) {

The selector sting #megamenu > * means: select all children of elements with an ID of megamenu.

But there's a better option. You could add these classes to the HTML instead of through the JS, or you could not add these classes or anything like them at all - see below, at the bottom of Step 2.

Checking children length is weird Once the children are selected, doing this:

// if the nav li contains x1 children it is a regular link
// if the nav li contains x2 children it is a megamenu item and we add the classes

if (sub.length === 2) {

is pretty strange. I think it would be much more natural for the parent elements to distinguish themselves by whether they're a container for more items or just contain a plain link. Maybe use a class name of subgroup for such parents. Then, rather than checking the length of the children, you could just change the selector string above to:

for (const child of document.querySelectorAll('#megamenu > .subgroup')) {

in order to iterate over the subgroups.

Step 2

Earlier in my development I learned that we shouldn't add eventListener inside a loop!

Listeners in a loop are just fine as long as you use modern syntax, usually. There is only one situation where adding listeners in a loop will fundamentally fail, which is if you're using var, which has unintuitive function scope instead of block scope:

// Bad:
for (var i = 0; i < elms.length; i++) {
  var elm = elms[i];
  elm.addEventListener('click', () => {
    console.log(elm, 'clicked');
  });
}

// Fix:
for (let i = 0 // ...

One should never be using var in source code anyway, nowadays, due in part to problems like this. Always use const or let instead. (Prefer const when possible, since it indicates reassignment will not occur.)

There is one situation in which adding listeners inside a loop can lead to difficult-to-manage code, which is if elements that need listeners are added dynamically. See https://stackoverflow.com/questions/203198/event-binding-on-dynamically-created-elements. In such a case, it's often more manageable to add a single listener to the parent instead of adding listeners to the children on pageload and adding listeners to each dynamically created child on creation.

But your situation does not fall into either of the above 2 categories. There's nothing wrong with adding lots of listeners (except, maybe, if you have 1000+ of them, or something like that - that might have a performance impact, I'm not sure). If you can change your Step 2 code so that it adds listeners in a loop instead of the repetitive if (event.target.matches('.megamenu-trigger-1')) { blocks, feel free to do that. Or, you could continue on with your event delegation approach, but with some improvements:

Use querySelector to select a single element instead of getElementsByClassName(..)[0]. For example, you can replace these:

document.getElementsByClassName('megamenu-sub-1')[0]

with

document.querySelector('.megamenu-sub-1')

Another benefit of querySelector is that it accepts a selector string, and selector strings are far more flexible than plain classes are tag names.

Or, rather than all of these classes, you could navigate from the clicked element instead. You could remove Step 1's code completely, only keeping the subgroup class name for menu parents. Then, on click, check to see if the target is the first child of a .subgroup - if it is, this is the equivalent of the trigger being clicked, so you can dynamically navigate to its next sibling and change its class. Replace all of Step 2's code with the following:

document.addEventListener('click', (e) => {
  const { target } = e;
  if (!target.matches('.subgroup > :first-child')) {
    return;
  }
  // A trigger element was clicked, so:
  e.preventDefault();
  const submenu = target.nextElementSibling;
  submenu.classList.remove('dn');
  submenu.classList.add('db', 'megamenu-sub-triggered');
});

The most important part is using .nextElementSibling to get to the clicked element's next sibling, instead of using all those classes.

Step 3

Collections are truthy. You have if (document.getElementsByClassName('megamenu-sub-triggered')) {, but getElementsByClassName returns a collection, and not a possibly falsey value, so the if check is redundant. If you want to check if any elements match the class, check the collection's length >= 1 instead. Or, even better...

Iteration is overkill. Instead, you can select the currently opened submenu element which has the db class with a single selector string. If it exists, and if the clicked element was not inside it, then reset its classes:

document.addEventListener('click', (event) => {
  const openSubmenu = document.querySelector('megamenu-sub-triggered');
  if (openSubmenu && !openSubmenu.contains(event.target)) {
    openSubmenu.classList.remove('db', 'megamenu-sub-triggered');
    openSubmenu.classList.add('dn');
  }
});

Multiple classes? It's odd to have 3 separate classes that get added or removed together. It would make more sense to change your CSS rules so that you can have just a single class instead, that gets toggled on or off. For example, you could have a submenu-open class, and then do:

submenu.classList.add('submenu-open');

or

openSubmenu.classList.remove('submenu-open');

without the classes of db, megamenu-sub-triggered, or dn at all.