1
\$\begingroup\$
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.

\$\endgroup\$
4
\$\begingroup\$

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)

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ You've got a bug there. You only have one variable content that is shared by all instances of the event handler, so that all buttons will only toggle the last dropdown, not its own one. \$\endgroup\$ – RoToRa Dec 2 '15 at 12:55
2
\$\begingroup\$

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

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for noticing my bug. I'm not sure I personally like the idea of traversing the DOM upward though (target.parentNode.querySelector); it means that you couldn't put a wrapper around openDropdown for purely styling reasons. One could fix it either by using an IIFE, or by binding the element to the listener function. I'll try an edit. \$\endgroup\$ – Katana314 Dec 2 '15 at 14:43
1
\$\begingroup\$

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;
});
|improve this answer|||||
\$\endgroup\$

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.