5
\$\begingroup\$

I made a simple dropdown menu which opens when clicked, and closes when the user clicks anywhere outside the menu. The following is the codebase:

function deactivateAllDropdownTriggers() {
   document.querySelectorAll('.dropdown-trigger.active').forEach(function(elem) {
        elem.classList.remove('active');
    }) 
}

function handleDropdownClicks(event) {
    if (event.target.matches('.dropdown-trigger')) {
        if (event.target.classList.contains('active')) {
            event.target.classList.remove('active');
        } else {
            deactivateAllDropdownTriggers();
            event.target.classList.add('active');
        }
    } else {
        if (!event.target.matches('.dropdown-menu *')) {
            deactivateAllDropdownTriggers();
        }
    }
}

document.addEventListener('click', handleDropdownClicks, false);
.dropdown {
    display: inline-block;
    position: relative;
}
.dropdown .dropdown-menu {
    display: none;
    position: absolute;
    -moz-box-shadow: 0 2px 4px rgba(0, 0, 0, 0.15);
    -webkit-box-shadow: 0 2px 4px rgba(0, 0, 0, 0.15);
    box-shadow: 0 2px 4px rgba(0, 0, 0, 0.15);
    width: auto;
    border: 1px solid rgba(0, 0, 0, 0.15);
    border-radius: var(--border-radius);
    background-color: #ffffff;
    z-index: 1;
}
.dropdown .dropdown-menu ul {
    padding: 0;
    margin: 0;
}
.dropdown .dropdown-menu ul > li {
    list-style: none;
    margin: 0;
}
.dropdown .dropdown-menu ul > li.dropdown-menu-content {
    padding: 0.6rem 1.2rem;
    white-space: nowrap;
}
.dropdown .dropdown-menu ul > li.dropdown-menu-divider {
    height: 1px;
    background-color: rgba(0, 0, 0, 0.05);
}
.dropdown .dropdown-menu ul > li > a {
    display: block;
    text-decoration: none;
    padding: 0.6rem 1.2rem;
    color: rgba(0, 0, 0, 0.8);
    cursor: pointer;
    white-space: nowrap;
    min-width: 12rem;
}
.dropdown .dropdown-menu ul > li > a:hover {
    background-color: rgba(0, 0, 0, 0.025);
}
.dropdown .dropdown-trigger.on-click.active + .dropdown-menu {
    display: block;
}
<div class="dropdown">
    <button class="dropdown-trigger on-click">Pick your weapon</button>
    <div class="dropdown-menu">
        <ul>
            <li class="dropdown-menu-content">
                Weapons
            </li>
            <li class="dropdown-menu-divider"></li>
            <li><a href="#">Sword</a></li>
            <li><a href="#">Lance</a></li>
            <li><a href="#">Axe</a></li>
            <li><a href="#">Bow</a></li>
        </ul>
    </div>
</div>

<div class="dropdown">
    <button class="dropdown-trigger on-click">Pick your class</button>
    <div class="dropdown-menu">
        <ul>
            <li class="dropdown-menu-content">
                Classes
            </li>
            <li class="dropdown-menu-divider"></li>
            <li><a href="#">Fighter</a></li>
            <li><a href="#">Archer</a></li>
            <li><a href="#">Thief</a></li>
            <li><a href="#">Ninja</a></li>
        </ul>
    </div>
</div>

One thing that concerns me is the open event listener. I could not create a system where an event listener is added to the document when a menu is opened, and the event listener is removed when the menu is closed. Perhaps one of you could help me with that.

Anyway, would appreciate a review of this code.

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

Overall the code looks fine. I just have a couple suggestions about the JavaScript and CSS.

JS

querySelectorAll vs getElementsByClassName

Generally document.getElementsByClassName will be quicker than document.querySelectorAll (see this post for more information) and the former also returns a live HTMLCollection so it wouldn't need to be queried each time. Bearing in mind that deactivateAllDropdownTriggers() looks for elements with both class names dropdown-trigger and active, only the latter is really needed. If active applies to other elements, then perhaps a name like active-dropdown would help narrow down elements. In order to iterate over the items in that collection, they would need to be put into an array - that can be done with the spread operator ...

const activeElements = document.getElementsByClassName('active');
function deactivateAllDropdownTriggers() {
  [...activeElements].forEach(elem => elem.classList.remove('active'));
}

CSS

Useless class on-click

It appears that the on-click class is only utilized in the last selector (i.e. .dropdown .dropdown-trigger.on-click.active + .dropdown-menu) but that class name could be removed since it doesn't appear to be used anywhere else

default values

Some rules set values to what should be the default values - e.g. margin: 0 for the unordered list and list items, and cursor: pointer for the anchors. Those shouldn't be needed unless there are other styles matching the selectors that would add different values - e.g. for other page contents or a browser/plugin stylesheet

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

Instead of adding the event listener to the document element you could add it to every element that has a class of dropdown-trigger. Then you could make use of javascripts classList toggle function. That way you don't need the if else statement in handleDropdownClicks.

Example:

const dropdownTriggers = document.querySelectorAll(".dropdown-trigger")
  dropdownTriggers.forEach( item => {
    item.addEventListener("click", () => {
      item.classList.toggle("active")
    });
  });
\$\endgroup\$
  • \$\begingroup\$ Your solution would not let the users close the dropdown menu by clicking outside of the dropdown menu though. \$\endgroup\$ – darkhorse Sep 16 at 9:57
  • \$\begingroup\$ Your absolutely right. Sorry, I overlooked it... \$\endgroup\$ – Dennis Fink Sep 16 at 9:59

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.