How to display hide/show menu using javascript?

Question

The code is working but not perfectly.

When I click "Test1", the result:

test -1

test -2

The problem is when I click "Test2"

I am getting same result when I click "Test1"

the result:

test -1

test -2

Expected Result:

test -3

test -4

Here is the HTML:

function show() {
  var x = document.getElementById("showw");
  if (x.style.display === "none") {
    x.style.display = "block";
  } else {
    x.style.display = "none";
  }
}

function expand() {
  var y = document.getElementById("show-list");

  if (y.style.display === "none") {
    y.style.display = "block";

  } else {
    y.style.display = "none";
  }
}

<h1 onclick="show()"> Menu </h1>
<nav id="showw" style="display: none;" onclick="expand()">
  <nav id="menuu"> Test1
    <nav id="show-list" style="display: none;">
      <p>test -1</p>
      <p>test -2</p>
    </nav>
  </nav>
  <nav id="menuu"> Test2
    <nav id="show-list" style="display: none;">
      <p>test -3</p>
      <p>test -4</p>
    </nav>
  </nav>
</nav>

Just FYI - All `id` attributes must have a unique value. If you want to have a common identifier for styling or other purposes, consider using a `class`. — Tom Faltesek, Nov 22 '19 at 21:54

Scott Marcus · Answer 1 · 2019-11-22T22:22:01.260

There's a great deal wrong with your code, starting with the fact that you have multiple elements that have the same id. IDs must be unique and this is why no matter which menu item you click, you always see the first set of choices - - the system only expects to find one element with a given id, so when it finds it, it stops looking for others.

From an HTML standpoint, you are using nav elements incorrectly. They should just be for sections that contain navigation.

Lastly, your code uses inline event handling and inline styles, both of which should be avoided as it makes the code more unreadable and promotes duplication of code. CSS and JavaScript should be separated out.

See comments inline below:

// Do your event binding in JavaScript, not in HTML
document.querySelector("h1").addEventListener("click", show);

// Find all the "menu" elements and loop over them
document.querySelectorAll(".menu").forEach(function(nav){
  // Assign a click event handler to each
  nav.addEventListener("click", expand);
});


function show(){
   // Get all the menu elements and loop over them
   document.querySelectorAll("div.menu").forEach(function(item){
      // Remove the "hidden" CSS class on the element, which will 
      // cause it to be shown. No need to set display:block -- 
      // just stop hiding it.
      item.classList.remove("hidden");
   });
}

function expand(){
  // Get the child nav of the clicked menu and toggle the use of the "hidden" class
  this.querySelector("nav").classList.toggle("hidden");
}

/* 
  Use CSS classes instead of inline style attributes.
  This avoids messy duplication of code and is much 
  easier to apply or remove the classes.
*/


/* Just add/or remove this class to hide or show an element */
.hidden { display:none; }

.pointer { cursor:pointer; }

<!-- 
     Notice how there is no CSS or JavaScript in the HTML
     and how much more clean and simple it is to read?
     Also, see how there are no IDs on anything, just
     classes to help the JavaScript and CSS know how to
     find the right elements?
-->
<h1 class="pointer">Menu</h1>
<div>
    <div class="menu hidden pointer">Test1
       <nav class="hidden">
         <ul>
          <li class="pointer">Test - 1</li> 
          <li class="pointer">Test - 2</li>
         </ul>
       </nav>
    </div> 
    <div class="menu hidden pointer">Test2
       <nav class="hidden">
         <ul>       
          <li class="pointer">Test - 3</li> 
          <li class="pointer">Test - 4</li>
         </ul>
       </nav>
    </div> 
</div>

I was going to answer something like this but, your answer is just perfect! — Carlos Franco, Nov 22 '19 at 22:16

score 0 · Answer 2 · answered Nov 22 '19 at 22:16

getElementById() find only the first much so test2 won't be affected. One other option to the already existing answers is to get the target element of the click event and make the changes on it.

change this line:

var y = document.getElementById("show-list");

by this line:

var y = event.target.firstElementChild;

How to display hide/show menu using javascript?

2 Answers2