Toggling the visibility of drop-down menu items

Question

var x = 0;
document.getElementById("1").addEventListener("click", function() {
    if ((x % 2) == 0) {
        document.getElementById("dropdown_1").style.display = 'block';
    } else {
        document.getElementById("dropdown_1").style.display = 'none';
    }
    x += 1;
});
var y = 0;
document.getElementById("2").addEventListener("click", function() {
    if ((y % 2) == 0) {
        document.getElementById("dropdown_2").style.display = 'block';
    } else {
        document.getElementById("dropdown_2").style.display = 'none';
    }
    y += 1;
});

I made a HTML-Page with a menu that has a drop-down function. But i wasnt able to write it in a proper code, i thought using a loop would be helpful but i didnt manage to do so.

Answer 1

If you are ever writing an element ID that contains a number, as opposed to a totally unique name, you should re-think how you're using them. In fact, if you want to take a totalitarian mental shortcut, just never use IDs (at least, until you're very experienced and understand their pros/cons). Most JavaScript frameworks allow you to easily query for all elements that use a certain class, and even if you aren't using a framework, it's not so hard to loop through the results of a selector. That said, it's possible the main difficulty in this instance is associating a dropdown button with dropdown content. If the HTML is up for modifying, here's a loose way that I would do it.

HTML:

<div class="dropdown">
   <button type="button" class="openDropdown">Open</button>
   <div class="dropdownContent">
   </div>
</div>

CSS:

.dropdownContent:not(.visible) {
  display: none;
}

JS:

var dropdowns = document.querySelectorAll('.dropdown');
for (var i = 0; i < dropdowns.length; i++) {
  var dropdown = dropdowns[i];
  var content = dropdown.querySelector('.dropdownContent');
  dropdown.querySelector('.openDropdown').addEventListener('click', function() {
    content.classList.toggle('visible');
  }
}

Part of the reason I used classes as opposed to direct styling is it's easier to toggle; but also, the other options for display: properties can often conflict with a developer's attempt to show/hide it. inline-block is very useful, but has previously been messed up for me because of code that was showing/hiding an element and overrode that.

EDIT: RoToRa found a bug in the above code - the closure generation for the addEventListener block will preserve the variable content, but that variable will change before the loop actually finishes, so in the end it will only apply to the last instance. (You won't notice the issue if you only have one dropdown) Here's my attempt at a fix: Note the arguments to the listener, and the .bind at the end.

var dropdowns = document.querySelectorAll('.dropdown');
for (var i = 0; i < dropdowns.length; i++) {
  var dropdown = dropdowns[i];
  var content = dropdown.querySelector('.dropdownContent');
  dropdown.querySelector('.openDropdown').addEventListener('click', function(content, evt) {
    content.classList.toggle('visible');
  }.bind(null, content);
}

Closures are very convenient automatic ways to preserve a variable with asynchronous programming, but bind is a more manual way that treats it as an argument. Another option would have been to turn the entire inside of the loop into a function, and run it immediately (creating a separate closure context that will exist once for each loop run)

Answer 2

Extending the solution of Katana314, which unfortunately has a bug. Instead of using a variable outside the event handler, it's better to access the clicked element directly from the event object passed to the handler:

var dropdowns = document.querySelectorAll('.dropdown');
for (var i = 0; i < dropdowns.length; i++) {
  var dropdown = dropdowns[i];
  dropdown.querySelector('.openDropdown').addEventListener('click', function(event) {
    event.target.parentNode.querySelector('.dropdownContent').classList.toggle('visible');
  });
}

Working example: http://codepen.io/anon/pen/pgzrVV

Answer 3

Don't manipulate style directly

Rather than manipulating the style values directly like this:

document.getElementById("dropdown_1").style.display = 'block';

It would be better to add and remove CSS classes, for example through the classList or className attributes. The reason is that it's best when the code is independent from formatting, styling, so that these responsibilities are cleanly separated.

Btw, jQuery could make HTML manipulations far easier, but if you want to keep a small footprint, there are other alternatives too, like zepto.js.

Don't repeat yourself

This piece of code is repeated many times, when it could be easily generalized:

document.getElementById(id).style.display = display;

Let's create some helper functions:

function setDisplay(id, display) {
    document.getElementById(id).style.display = display;
}

function show(id) {
    setDisplay(id, 'block');
}

function hide(id) {
    setDisplay(id, 'none');
}

Now the code can be slightly simplified:

var x = 0;
document.getElementById("1").addEventListener("click", function() {
    if ((x % 2) == 0) {
        show("dropdown_1");
    } else {
        hide("dropdown_1");
    }
    x += 1;
});
var y = 0;
document.getElementById("2").addEventListener("click", function() {
    if ((y % 2) == 0) {
        show("dropdown_2");
    } else {
        hide("dropdown_2");
    }
    y += 1;
});