0
\$\begingroup\$

I've created a menu that allows you to display the list when clicking on the menu icon. I wonder whether this can be improved somehow - can anyone help improve this?

Also, initially I created it with the event passed into the script to prevent it from scrolling to the top when clicking on the link. I then changed it to an ES6 function - does it not need the event passed in and calling e.preventDefault()?

Thank you. The code is as follows:

  const cont = document.querySelector('.cont');
  const hamburger = document.querySelector('#hamburger');
  
  // const toggleMenu = (e) => {
  //   e.preventDefault();
  //   cont.classList.toggle('open');
  // }
  const toggleDisplay = () => cont.classList.toggle('open');
  
  hamburger.addEventListener('click', toggleDisplay);
.cont {
  position: relative;
}
a {
  font-size: 3em;
}
a:hover {
  cursor: pointer;
}
a::after {
  content: '\002b';
}
.open a::after {
  content: '\00d7';
  color: red;
}
.open ul {
  display: flex;
}
ul {
  all: unset;
  display: none;
  position: absolute;
  background: #0ff;
  padding: .5em;
  min-width: 100px;
  top: 100%;
  left: 0;
  flex-direction: column;
  text-align: center;
}
li {
  all: unset;
  width: 100%;
}
li:not(:first-child) {
  margin-top: .5em;
  padding-top: .5em;
  border-top: 1px solid #ccc;
}
<div class='cont'>
  <a id='hamburger'></a>
  <ul>
    <li>First</li>
    <li>Second</li>
    <li>Third</li>
  </ul>
</div>

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Here are a few suggestions:

  • '.cont' is a bit vague - I suppose it means container but try to avoid meaningless abbreviations
  • Be careful with all: unset. It might have unexpected consequences cross-browser.
  • I would stick to rem for margin/paddings and only use em for fontsizes. There is a lot of articles explaining the difference if you perform a simple google search
  • You could make it behave a little more smoothly if you had a CSS transition
  • Instead of a wrapping div this could potentially be a nav element if it is part of some navigation. Always try to use semantically correct tags
  • I don't think you want an a tag here as you are not linking to anything. Maybe a button would be better?

As for your question both of your functions are ES6 functions (fat-arrow functions).

In the example you commented out you are using explicit return while you are using implicit return in the latter.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for your feedback. A few questions: 1. What side-effects could all: unset have? 2. I read that the opposite is true, and that em is perfect for padding/margin as it's based on the inherited font size, and rem is ok for font sizes - research seemed subjective but generally agreeable on em for margins/padding 3. What's wrong with the a tag? It's good for accessibility as it's selectable when pressing tab (although so are buttons) 4. True, but does it actually need e.preventDefault()? \$\endgroup\$ Feb 27 at 20:41
  • \$\begingroup\$ 1. You can check compatibility on "caniuse.com".. It might also impact accessibility on some fields. In your case I guess you are really just trying to reset the list-style-type. 2. em is relative to the nearest parent while rem is to the root font-size. That's why padding/margins with em could lead to more inconsistent designs imo. Not sure where you found that consensus :). 3. It's semantically incorrect. You should use a tags for links. You can get the same accessibility with a button. 4. No there is no default action to prevent if you don't have any href value. \$\endgroup\$
    – J.Kirk.
    Feb 27 at 21:10
  • \$\begingroup\$ Use tags that best convey intent - ditto. I'd have upvoted even if that was the only nugget in this answer. \$\endgroup\$
    – radarbob
    Feb 28 at 19:52

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.