CODEWITHSUNDEEP
javascripthtmlcssonclick
cokicah505
cokicah505

Reputation: 41

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

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>

Upvotes: 1

Views: 4316

Answers (2)

EGC
EGC

Reputation: 1789

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:

  1. Give your button a unique identifier (id)
  2. Listen to the click event associated with that unique id
  3. 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.

Upvotes: 2

CertainPerformance
CertainPerformance

Reputation: 371193

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>

Upvotes: 2

Related Questions