0
\$\begingroup\$

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"; }
            
        }

     }

}
Improve this question
\$\endgroup\$
5
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – Sumurai8
    Dec 31, 2020 at 19:42
  • 1
    \$\begingroup\$ @Sumurai8 What's wrong with it? \$\endgroup\$
    – Mast ♦
    Dec 31, 2020 at 22:06
  • \$\begingroup\$ The site looks empty. Are you sure it's ready for review? Is this part of an actually working site? \$\endgroup\$
    – Mast ♦
    Dec 31, 2020 at 22:24
  • \$\begingroup\$ 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. \$\endgroup\$
    – Sumurai8
    Jan 1, 2021 at 10:58
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – Ganesh R
    Jan 2, 2021 at 4:19

1 Answer 1

Reset to default
1
\$\begingroup\$

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 &#9776; ("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)
Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Thanks very much.. I'll amend accordingly. Sorry was busy with other stuff only saw today. \$\endgroup\$
    – Ganesh R
    Feb 3, 2021 at 3:49

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.