How do I replace onclick with a better alternative in vanilla javascript?

Question

I wrote the following code and it works, but people are telling me not to use the onclick. How do I implement an alternative in a case like this? I'm replacing the "hidden" class with the "block" class, and vice versa. Would I be wrong to use this in production, or is there a better alternative as people have mentioned to me.

function myFunction() {
  var element = document.getElementById("menu");
  if (element.classList.contains("hidden")) {
    element.classList.replace("hidden", "block");
  } else if (element.classList.contains("block")) {
    element.classList.replace("block", "hidden");
  }
}

.hidden {
  display: none;
}

.block {
  display: block;
}

.icon {
  fill: currentColor;
  height: 24px;
  width: 24px;
}

<button onclick="myFunction()">
        <svg class="icon" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg"><title>Menu</title><path d="M0 3h20v2H0V3zm0 6h20v2H0V9zm0 6h20v2H0v-2z"/></svg>
      </button>

<div id="menu" class="hidden">
  <p>I'm no longer hidden.</p>
</div>

score 2 · Accepted Answer · answered Jan 09 '20 at 00:14

The problem with inline click attributes is that their scoping rules are very strange and unintuitive, and they require global pollution. Select the element using Javascript and use addEventListener instead.

You can also use classList.toggle instead of alternating between two different classes; the .block class isn't useful because a <div>'s ordinary display is already block.

document.querySelector('button').addEventListener('click', () => {
  document.getElementById("menu").classList.toggle('hidden');
});

.hidden {
  display: none;
}
.icon {
  fill: currentColor;
  height: 24px;
  width: 24px;
}

<button>
    <svg class="icon" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg"><title>Menu</title><path d="M0 3h20v2H0V3zm0 6h20v2H0V9zm0 6h20v2H0v-2z"/></svg>
  </button>

<div id="menu" class="hidden">
  <p>I'm no longer hidden.</p>
</div>

If you have multiple buttons and can't easily target the one you want to attach the listener to, then give it a class so you can target it with querySelector:

document.querySelector('.hamburger').addEventListener('click', () => {
  document.getElementById("menu").classList.toggle('hidden');
});

.hidden {
  display: none;
}
.icon {
  fill: currentColor;
  height: 24px;
  width: 24px;
}

<button class="hamburger">
    <svg class="icon" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg"><title>Menu</title><path d="M0 3h20v2H0V3zm0 6h20v2H0V9zm0 6h20v2H0v-2z"/></svg>
  </button>

<div id="menu" class="hidden">
  <p>I'm no longer hidden.</p>
</div>

I see. One question though: document.querySelector('button')..., will this not trigger the toggle if I had other buttons on the page as well? Is there a way to querySelector() by id, or am I thinking about this in the wrong way? — cokicah505, Jan 09 '20 at 00:18
That's why I said *If you have multiple buttons and can't easily target the one you want to attach the listener to, then give it a class so you can target it with `querySelector`* - see the second snippet, give the button a class. — CertainPerformance, Jan 09 '20 at 00:18
document.querySelector('#menu-icon').addEventListener... seems to work as well. Is it ok/best practice to use querySelector with an id (instead of class) as well? — cokicah505, Jan 09 '20 at 00:25
I prefer to avoid IDs because elements with IDs create properties on the global object, which results in unnecessary global pollution, and can occasionally result in confusing bugs, so I prefer classes instead — CertainPerformance, Jan 09 '20 at 00:29

score 2 · Answer 2 · answered Jan 09 '20 at 00:16

See this answer on Why is using onClick() in HTML a bad practice?.

Whereby Unobtrusive JavaScript is discussed in this fashion:

The advantages are behaviour (Javascript) is separated from presentation (HTML)

no mixing of languages

you're using a javascript framework like jQuery that can handle most cross-browser issues for you

You can add behaviour to a lot of HTML elements at once without code duplication

The good news is that you don't need to do much to retrofit your code, you just need to:

Give your button a unique identifier (id)
Listen to the click event associated with that unique id
Run myFunction when this is clicked

Here is your code but retrofitted:

<!doctype html>
<html lang="en-US">

  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1" />
    <title>Untitled</title>
    <style>
      .hidden {
        display: none;
      }

      .block {
        display: block;
      }

      .icon {
        fill: currentColor;
        height: 24px;
        width: 24px;
      }

    </style>
  </head>

  <body>
    <button id="btnHideShow">
      <svg class="icon" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg">
        <title>Menu</title>
        <path d="M0 3h20v2H0V3zm0 6h20v2H0V9zm0 6h20v2H0v-2z" />
      </svg>
    </button>

    <div id="menu" class="hidden">
      <p>I'm no longer hidden.</p>
    </div>

    <script>
      function myFunction() {
        var element = document.getElementById("menu");
        if (element.classList.contains("hidden")) {
          element.classList.replace("hidden", "block");
        } else if (element.classList.contains("block")) {
          element.classList.replace("block", "hidden");
        }
      }

      document.getElementById("btnHideShow").addEventListener("click", myFunction);

    </script>
  </body>

</html>

Here's a JSFiddle to view as well.

That explains a lot, thanks. It's taking me a while to work through learning the basics. I just want to make sure I'm generally following best practices as I learn. — cokicah505, Jan 09 '20 at 00:27

How do I replace onclick with a better alternative in vanilla javascript?

2 Answers2