1
\$\begingroup\$

Here is my first attempt to create an simple drop down menu with CSS and Java Script. This is the HTML code.

 <div class="dropdown menu1">
    <button>Menu 1</button>
    <div class="dropdown-content">
        <a href="#">Option 1</a>
        <a href="#">Option 2</a>
    </div>
 </div>
<div class="dropdown menu2">
    <button>Menu 2</button>
    <div class="dropdown-content">
        <a href="#">Option 1</a>
    </div>
</div>
<div class="dropdown menu3">
    <button>Menu 3</button>
    <div class="dropdown-content">
        <a href="#">Option 1</a>
        <a href="#">Option 2</a>
        <a href="#">Option 3</a>
    </div>
</div>

CSS

div.dropdown {
    display: inline-block;
}
div.dropdown button {
    background-color:#fff;
    border:0px;
    color:#424242;
    cursor:pointer;
    padding:5px 0 5px 0;
}
div.menu1, div.menu2 {
    margin:0 0 0 20px;
}
div.menu1 button {
    border-bottom:2px solid #00BCD4;
}
div.menu2 button {
    border-bottom:2px solid #4CAF50;
}
div.menu3 button {
    border-bottom:2px solid #9FA8DA;
}
div.dropdown-content {
    display: none;
}
div.dropdown-content-active {
    margin-top:5px;
    display: block;
    background-color: #fff;
    border:1px solid #EEEEEE;
    box-shadow: 0px 0px 2px 1px #dcdcdc;
    position:absolute;
    border-radius:3px;
    z-index: 10;
}
div.dropdown-content a {
    display: block;
    width:100%;
    text-decoration: none;
    color:#3d3d3d;
    padding:4px;
    box-sizing: border-box;
    font-size:0.9em;
}
div.dropdown-content a:hover {
    background-color: #f0f0f0;
}

JavsScript

document.addEventListener('DOMContentLoaded', function() {
    [].forEach.call(document.querySelectorAll('div.dropdown button'), function(dropdown) {
        dropdown.addEventListener('click', function() {

            if(getComputedStyle(this.nextSibling).display === 'none') {

                [].forEach.call(document.querySelectorAll('div.dropdown div.dropdown-content-active'), function(dropdown_active) {
                    dropdown_active.classList.remove('dropdown-content-active');
                });

                this.nextSibling.classList.add('dropdown-content-active');
            } else {
                this.nextSibling.classList.remove('dropdown-content-active');
           }

        });
    });
});

The JavaScript is really simple but I am not sure that this is the best way I can achieve this. Would appreciate your comments.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Debugging:

When running your code in Firefox, I get the following error:

TypeError: Argument 1 of Window.getComputedStyle does not implement interface Element.

This is because button.nextSibling() is a text node. Use nextElementSibling() instead.

Suggestions:

  • I recommend attaching an "active" class to the dropdown itself instead of its content. The semantics are clearer and it provides you with more flexibility. E.g. you could style an active toggle button differently.

  • Also, instead of querying the computed display style (slow!) of the button in order to infer whether a dropdown is active or inactive via

    if (getComputedStyle(this.nextSibling).display === 'none') {
      ...
    }
    

    you could simply check for the dropdown's "active" class via

    if (dropdown.classList.contains('.active') {
      ...
    } 
    

    which is faster and agnostic to the specific CSS properties of an "active" dropdown button.

  • Additionally, instead of relying on a fixed order of dropdown toggle button and dropdown content by querying for the dropdown button first and then getting the content via nextSibling() or nextElementSibling(), I recommend querying for the dropdown first and then separately querying its children for a dropdown-toggle and the dropdown-content.

  • Finally, I recommend simplifying the deeply nested structure of your code by moving the reusable deeply nested code into a toggleDropdown function.

Style:

While some people consider the following idiomatic:

[].forEach.call(document.querySelectorAll('...'), function(element) {
  ...
});

I prefer using the simpler for ... of iteration:

for (let element of document.querySelectorAll('...')) {
  ...
}

Assigning the query results to a variable with a self-documenting name might improve readability:

let elements = document.querySelectorAll('...');
for (let element of elements) {
  ...
}

Enhancements:

You might want to listen to the "blur" event and deactivate the dropdown when its focus is transferred to an outside element.

function toggleDropdown(dropdown) {
  if (dropdown.classList.contains('active')) {
    dropdown.classList.remove('active');
  } else {
    let activeDropdowns = document.querySelectorAll('.dropdown.active');
    for (let activeDropdown of activeDropdowns) {
      activeDropdown.classList.remove('active');
    }
    dropdown.classList.add('active');
  }
}

let dropdowns = document.querySelectorAll('.dropdown');
for (let dropdown of dropdowns) {
  
  // Toggle when dropdown toggle clicked:
  let toggle = dropdown.querySelector('.dropdown-toggle');
  toggle.addEventListener('click', function() {
    toggleDropdown(dropdown);
  });
  
  // Deactivate when focus leaves dropdown:
  dropdown.addEventListener('blur', function(event) {
    if (!this.contains(event.relatedTarget)) {
      this.classList.remove('active')
    }
  }, true);
}
.dropdown {
  display: inline-block;
}

.dropdown .dropdown-toggle {
  background-color: #fff;
  border: 0;
  color: #424242;
  cursor: pointer;
  padding: 5px 0 5px 0;
}

.dropdown .dropdown-content {
  display: none;
}

.dropdown.active .dropdown-content {
  margin-top: 5px;
  display: block;
  background-color: #fff;
  border: 1px solid #eee;
  box-shadow: 0 0 2px 1px #dcdcdc;
  position: absolute;
  border-radius: 3px;
  z-index: 10;
}

.dropdown .dropdown-content a {
  display: block;
  width: 100%;
  text-decoration: none;
  color: #3d3d3d;
  padding: 4px;
  box-sizing: border-box;
  font-size: 0.9em;
}

.dropdown .dropdown-content a:hover {
  background-color: #f0f0f0;
}

.dropdown.menu1,
.dropdown.menu2 {
  margin: 0 0 0 20px;
}

.dropdown.menu1 .dropdown-toggle {
  border-bottom: 2px solid #00BCD4;
}

.dropdown.menu2 .dropdown-toggle {
  border-bottom: 2px solid #4CAF50;
}

.dropdown.menu3 .dropdown-toggle {
  border-bottom: 2px solid #9FA8DA;
}
<div class="dropdown menu1">
    <button class="dropdown-toggle">Menu 1</button>
    <div class="dropdown-content">
        <a href="#">Option 1</a>
        <a href="#">Option 2</a>
    </div>
 </div>
<div class="dropdown menu2">
    <button class="dropdown-toggle">Menu 2</button>
    <div class="dropdown-content">
        <a href="#">Option 1</a>
    </div>
</div>
<div class="dropdown menu3">
    <button class="dropdown-toggle">Menu 3</button>
    <div class="dropdown-content">
        <a href="#">Option 1</a>
        <a href="#">Option 2</a>
        <a href="#">Option 3</a>
    </div>
</div>

Edit:

You could move the declaration of both event listeners out of the loop body. However, the dropdown toggle's 'click' event listener needs access to the dropdown element it belongs to. So you would still need a closure or alternatively bind the event listener's this to the dropdown:

function onDropdownBlur(event) {
  if (!this.contains(event.relatedTarget)) {
    this.classList.remove('active')
  }
}

function onDropdownToggle(event) {
  toggleDropdown(this);
}

let dropdowns = document.querySelectorAll('.dropdown');
for (let dropdown of dropdowns) {

  // Toggle when dropdown toggle clicked:
  let toggle = dropdown.querySelector('.dropdown-toggle');
  toggle.addEventListener('click', onDropdownToggle.bind(dropdown));

  // Deactivate when focus leaves dropdown:
  dropdown.addEventListener('blur', onDropdownBlur, true);
}
\$\endgroup\$
4
  • \$\begingroup\$ jsHint is giving me the following suggestion: Don't make functions inside a loop. How can we avoid this? \$\endgroup\$
    – Luchezar
    Commented Apr 27, 2017 at 13:17
  • \$\begingroup\$ @Luchezar It's probably because the 'click' and 'blur' event listener are declared within the for ... of loop body. You cannot move the 'click' listener out of the loop body as it needs to close over the dropdown variable - each event handler is a distinct closure. Turning the loop back into a [].forEach won't change that fact. If you really wish to move the 'click' event handler declaration out of the loop body, you need to look up the toggle button's (this) parent's to re-discover the dropdown each time you click. \$\endgroup\$
    – le_m
    Commented Apr 27, 2017 at 13:28
  • 1
    \$\begingroup\$ @Luchezar I appended an edit to my review in reply to your comment, please have a look. \$\endgroup\$
    – le_m
    Commented Apr 27, 2017 at 13:39
  • \$\begingroup\$ Thank you very much for your time! I will examine both your codes and try to improve my future writings. \$\endgroup\$
    – Luchezar
    Commented Apr 27, 2017 at 13:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.