Hamburger menu with Javascript - clearing state

Question 1

I wanted a menu that is always a hamburger, regardless of screen size. Plus it has to be activated by click, not hover.

Any comments / review will be appreciated.

Pls let me know if this is not worded properly / off topic etc.

Please see Codepen

<!DOCTYPE html>
<html>
 <head>
 <meta charset="utf-8">
 <meta name="viewport" content="width=device-width, initial-scale=1">
 <title>Sample website</title>
 <link rel="stylesheet" type="text/css" href="css/test.css"> </head>
 <body>
 <div id="menuBar">
 <div class = "menuHamburger" onclick = "hideAllButFirstChild('menuBar')"> &#9776; </div>
 <ul id='menu1'>
 <li onclick="hideAllButFirstChild('menu1')">Menu 1</li>
 <li>Menu 1-1</li>
 <li>Menu 1-2</li>
 <li>Menu 1-3</li>
 <li>Menu 1-4</li>
 </ul>
 <ul id='menu2'>
 <li onclick="hideAllButFirstChild('menu2')">Menu 2</li>
 <li>Menu 2-1</li>
 <li>Menu 2-2</li>
 <li>Menu 2-3</li>
 <li>Menu 2-4</li>
 </ul>
 <ul id='menu3'>
 <li onclick="hideAllButFirstChild('menu3')">Menu 3</li>
 <li>Menu 3-1</li>
 <li>Menu 3-2</li>
 <li>Menu 3-3</li>
 <li>Menu 3-4</li>
 </ul>
 </div>
 <script src="js/test.js"></script>
 </body>
</html>
#menuBar > ul {
 display: none;
}
ul {
 list-style-type: none;
 margin: 0;
 padding: 0;
 width: 10em;
}
li {
 list-style-type:none;
 margin: 1px;
 padding: 1px;
}
li:first-child {
 background: grey;
 color: #fff;
}
li:not(:first-child) {
 display: none;
}
.menuHamburger {
 font-size: 2em;
}
function hideAllButFirstChild(parentId) {
 
 var children = document.getElementById(parentId).children;
 for (var i=0; i < children.length; i++) {
 if (children[i].style.display=='' || children[i].style.display=='none') {
 children[i].style.display="block";
 // but set its children to none?
 } else {
 // block all except first
 if (i != 0) { children[i].style.display="none"; }
 
 }
 }
}

Question 2

I've flagged this question for closure, because the code does not appear to work as intended. This is required to get a code review. To get help with non-working code, ask an on-topic question on Stackoverflow instead.

Question 3

@Sumurai8 What's wrong with it?

Question 4

The site looks empty. Are you sure it's ready for review? Is this part of an actually working site?

Question 5

The hideAllButFirstChild function does not seem to do what the function name claims it should do. It seems at most to be a toggle of sorts, but the "hide all" functionality seems to be missing or just doesn't work. The function also contains a question in a comment. Besides that the menu seems to be a prototype of sorts, missing critical things like... links... for example. And while that does seem like a minor gripe, links being clickable by nature cause all kinds of issues that are not even touched yet.

Question 6

Hi, it's not a complete site just a menu and I wanted comments on its implementation. If you see the code pen, the menu expands and collapses on click. That's the part I would like reviewed for better approach or general advise etc.

Question 7

HTML

The formating could be a bit cleaner (</head> on its own line, superfluous spaces around =, content of <div id="menuBar"> not indented).
on... attributes shouldn't be used. Event handlers should be assigned in the script.
I don't believe ☰ ("Trigram For Heaven") is a suitable character to use for a menu icon.
The submenus aren't structured properly in my opinion:
- The clickable header entries shouldn't be list items themselves, but separate elements before the ul.
- Also they should be an interactive elements such as <button> or <a>.
- If you would also wrap all the submenus in a separate item with the hamburger button before it, then you could simplify the JavaScript so that it simply toggles that single element, which spares you the loop and the exception of the first element.
- Alternatively you could even consider using the <details> element, which has the toggling built in, requiring no JavaScript at all.

JavaScript

Instead of passing the ID of the parent element into the function you could get the clicked element (and thus its parent) from the event object. Example:

// Assign event handler in JavaScript instead of using the HTML attribute
document.querySelector(".menuHamburger").addEventListener("click", hideAllButFirstChild);
function hideAllButFirstChild(event) {
 // TODO Add null checks.
 const children = event.target.parent.children;
 // ...
}

And as seen above use const or let instead of var for variables.
Start the loop at i = 1 to skip the first element instead of checking inside the loop.
Instead of manipulating style in JavaScript set/remove a class (or, better, in this case the hidden attribute)

Question 8

Thanks very much.. I'll amend accordingly. Sorry was busy with other stuff only saw today.

RoToRa RoToRa 11.6k1 gold badge24 silver badges51 bronze badges · Accepted Answer · 2021-01-15 15:57:42Z

HTML

The formating could be a bit cleaner (</head> on its own line, superfluous spaces around =, content of <div id="menuBar"> not indented).
on... attributes shouldn't be used. Event handlers should be assigned in the script.
I don't believe ☰ ("Trigram For Heaven") is a suitable character to use for a menu icon.
The submenus aren't structured properly in my opinion:
- The clickable header entries shouldn't be list items themselves, but separate elements before the ul.
- Also they should be an interactive elements such as <button> or <a>.
- If you would also wrap all the submenus in a separate item with the hamburger button before it, then you could simplify the JavaScript so that it simply toggles that single element, which spares you the loop and the exception of the first element.
- Alternatively you could even consider using the <details> element, which has the toggling built in, requiring no JavaScript at all.

JavaScript

Instead of passing the ID of the parent element into the function you could get the clicked element (and thus its parent) from the event object. Example:

// Assign event handler in JavaScript instead of using the HTML attribute
document.querySelector(".menuHamburger").addEventListener("click", hideAllButFirstChild);
function hideAllButFirstChild(event) {
 // TODO Add null checks.
 const children = event.target.parent.children;
 // ...
}

And as seen above use const or let instead of var for variables.
Start the loop at i = 1 to skip the first element instead of checking inside the loop.
Instead of manipulating style in JavaScript set/remove a class (or, better, in this case the hidden attribute)

Thanks very much.. I'll amend accordingly. Sorry was busy with other stuff only saw today.

Stack Exchange Network

Hamburger menu with Javascript - clearing state

1 Answer 1

HTML

JavaScript

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

Hamburger menu with Javascript - clearing state

1 Answer 1

HTML

JavaScript

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions