JavaScript dropdown menu

Question

In the past when I needed something like a dropdown menu, it was really easy to toggle the appropriate CSS classes with jQuery.

Here is my first attempt to do the same thing without any dependencies:

// create object containing all .dropdown elements
var dropdown = document.getElementsByClassName("dropdown");
// loop through "dropdown"
for (var d = 0; d < dropdown.length; d++) {
 // send each element to the "dropdownListen" function to listen for click
 dropdownListen(dropdown[d]);
}
function dropdownListen(elem) {
 elem.onclick = function(e) {
 // go no farther if inside the dropdown was clicked
 if (!e.target.matches('.dropdown-menu-item, .dropdown-body, p')) {
 // If the current dropdown is not already open, check all of the others and close any that are found
 if (!this.classList.contains('shown')) {
 // create object containing all .dropdown elements, again
 var dropdowns = document.getElementsByClassName("dropdown");
 // loop through "dropdowns"
 for (var d = 0; d < dropdowns.length; d++) {
 var openDropdown = dropdowns[d];
 // remove class "shown" if any open dropdown is found
 if (openDropdown.classList.contains('shown')) {
 openDropdown.classList.remove('shown');
 }
 }
 }
 // toggle the class of the dropdown that was clicked
 this.classList.toggle('shown');
 }
 };
}
document.onclick = function(e) {
 // close any open dropdowns when clicking anywhere else on the document
 if (!e.target.matches('.dropdown-toggle, .dropdown-menu-item, .dropdown-body, p')) {
 // create object containing all .dropdown elements, again
 var dropdowns = document.getElementsByClassName("dropdown");
 // loop through "dropdowns"
 for (var d = 0; d < dropdowns.length; d++) {
 var openDropdown = dropdowns[d];
 // remove class "shown" if any open dropdown is found
 if (openDropdown.classList.contains('shown')) {
 openDropdown.classList.remove('shown');
 }
 }
 }
}

.button-group {
 display: inline-block;
}
.button {
 background: lightblue;
 border: 1px solid darkblue;
 padding: 5px 10px;
 display: inline-block;
 cursor: default;
}
.dropdown-body {
 position: absolute;
 border: 1px solid #ddd;
 min-width: 100px;
 box-shadow: 1px 1px 15px 0px #f0f0f0;
 border-radius: 3px;
 padding: 3px 0px;
 background-color: #fff;
 left: auto;
 right: 0;
 /* Align to Right by default */
}
.dropdown-body .dropdown-menu-item {
 padding: 5px;
 cursor: default;
 display: block;
 text-decoration: none;
 color: #333;
}
.dropdown-body .dropdown-menu-item:hover {
 background-color: #08c;
 color: #fff;
}
.dropdown {
 position: relative;
}
.dropdown.dropdown-left .dropdown-body {
 right: auto;
 left: 0;
}
.dropdown .dropdown-body {
 display: none;
}
.dropdown.shown .dropdown-body {
 display: block;
}

<div class="button-group dropdown">
 <div class="dropdown-toggle button">Default Dropdown</div>
 <div class="dropdown-body">
 <div class="dropdown-menu-item">Item 1</div>
 <div class="dropdown-menu-item">Item 2</div>
 <div class="dropdown-menu-item">Item 3</div>
 <div class="dropdown-menu-item">Item 4</div>
 <div class="dropdown-menu-item">Item 5</div>
 </div>
</div>
<div class="button-group dropdown dropdown-left">
 <div class="dropdown-toggle button">Left Dropdown</div>
 <div class="dropdown-body">
 <a href="http://google.com" class="dropdown-menu-item" target="_blank">Google.com</a>
 <div class="dropdown-menu-item">Item 2</div>
 <div class="dropdown-menu-item">Item 3</div>
 <div class="dropdown-menu-item">Item 4</div>
 <div class="dropdown-menu-item">Item 5</div>
 </div>
</div>
<div class="button-group dropdown dropdown-left">
 <div class="dropdown-toggle button">Some other content</div>
 <div class="dropdown-body">
 <p>Lorem ipsum dolor sit amet, consectetur adipisicing elit. Blanditiis, dolore.</p>
 </div>
</div>

Features:

• Only one dropdown can be open at a time
• Clicking inside does not close it
• Clicking outside closes any open dropdown

Questions/Concerns:

• Possible unnecessary duplication of code
• You have to individually name every element that could be inside a dropdown to prevent it from closing when clicking inside.
• This is my first attempt at writing JavaScript without a library or framework. Am I on the right track? Am I following common best practices?

• albert: you are on the right track javascript wise, although most of this can easily be done with css. as for your markup...its on the wrong track. ditch the divs, use lists and list items, as well as anchors for buttons (seriously, not divs). looks good tho.
• Jack: @albert Thanks for the advice. CSS is doing most of the work right now. The javascript is really just adding/removing the shown class. As for the markup, that is an easy change (I just used divs for this example for some reason).
• albert: np...the js makes it smoother but you can simply hide/show on :hover,:active,:focus in regards to the nested lists. here's a quick fiddle using unordered lists wrapped in a nav element jsfiddle.net/jalbertbowdenii/mdufqLtc/16 but of course we are talking semantics, so whatever works for me may not work for you ;)
• Jack: @albert I just remembered, the main reason I used divs was so that any content (not just menu items) could be in the dropdown.
• albert: lol....you can put whatever you want in lists...but again, its just semantics. whatever works, ya know?

Answer 1

After looking at your code, I don't see any real issues with your CSS and HTML. If it works it, then nothing else to do there. However, there are a couple things you can do to simplify your JavaScript code.

Since you are using classlist, you are supporting IE10+ (unless you add a polyfill). So this means you do not need to use the old onclick events and you can use the newer addEventListener and querySelectorAll to simplify your code.

My first suggestion is always to wrap your code in an IIFE to provide yourself a private scope. This will keep your code out of the global scope and help prevent issues with other peoples code. Its easy to setup:

(function( window, document ) {
 //code goes here
})( window, document );

I am also passing in local references to the global window and document objects for a ever-so-slight performance boost.

You also select your drop downs multiple times. So we can just create a single variable in our private scope to hold this selection. That way we can get the elements once ( on load) and not worry about it further.

(function( window, document ) {
 var dropdown;
 function init() {
 dd = document.querySelectorAll('.dropdown');
 }
 // other code goes here
 //Equivalent to jQuery document.ready
 document.addEventListener('DOMContentLoaded', init, false);
})( window, document );

Another thing that is adding unneeded complexity is checking to see if the class is on an element before removing it. We can just make the call to remove the class. If it has the class, it's removed. If not, no harm no foul.

dropdown.classList.remove('shown');

Also since we don't care about the order in which this is done, we can use a while loop. It is typically the fastest ways to loop in JavaScript.

var len = dropdown.length;
while(len--) {
 dropdown[len].classList.remove('shown');
}

Now we only need to create one function to actually handle the click event:

function handleClickEvent() {
 if (!e.target.matches('.dropdown-menu-item, .dropdown-body, p')) {
 var len = dropdown.length;
 while(len--) {
 dropdown[len].classList.remove('shown');
 }
 this.classList.add('shown');
 }
}

So putting that all together, you get something like this:

(function( window, document ) {
 var dropdown;
 function handleClickEvent() {
 if (!e.target.matches('.dropdown-menu-item, .dropdown-body, p')) {
 var len = dropdown.length;
 while(len--) {
 dropdown[len].classList.remove('shown');
 }
 this.classList.add('shown');
 }
 }
 function init() {
 dropdown = document.querySelectorAll('.dropdown');
 var len = dropdown.length;
 while(len--) {
 dropdown[len].addEventListener('click', handleClickEvent, false);
 }
 }
 document.addEventListener('DOMContentLoaded', init, false);
})( window, document );

• Jack: Very helpful answer. One question: When trying this, I get a console error: dropdown.addEventListener is not a function. Does dropdown have to be looped over coming from querySelectorAll?
• Gary Storey: You need to loop through each selected element like dropdown[len].adEventListener like in the loop above. I'll edit the answer
• Jack: There still seems to be an issue somewhere. Here is a fiddle.
• Gary Storey: Accidentally removed the DOMContentLoaded event listener
• Gary Storey: Here is a pen

Answer 2

1. First off, you have comments over many lines in your code. For example, comments like // toggle the class of the dropdown that was clicked, or // loop through "drodown" are completely useless. From looking at the code, it's clear as to what that line does. Comments like these should be removed.
2. You shouldn't be too concerned about duplication in your code. The only major place where I see this is your HTML code, but, it's HTML, so that's kind of unavoidable.
3. I like how you've done this without a framework of library like JQuery, and you still have pretty readable code. I would recommend using frameworks/libraries like JQuery, as they generally make life a whole lot easier.
4. Finally, just for readablility, I'd recommend separating out the styles for each of your HTML classes/elements/ids. It's just makes things slightly easier to read.

• Jack: Thanks. The comments by the way were purely for clarity and to show my understanding of how it works. They would be removed otherwise.
• Ethan Bierlein: @Jack Ah I see now.