CODEWITHSUNDEEP
javascriptjquery
Kyle D. Fiala
Kyle D. Fiala

Reputation: 63

Trying to understand this toggleClass() function

The function in question:

function toggleElementClass(element, className) {
if (element.className.indexOf(className) > -1) {
    element.className = element.className.replace(new RegExp(className, 'g'), '');
} else {
    element.className += " " + className;
}}

I'm trying to identify issues with this code. I've had experience with jQuery and JavaScript here and there, but I cannot seem to come to a solid conclusion with what I've seen so far. I've seen a lot of examples using the current .toggleClass() function from jQuery but none that help me analyze the code above.

One problem I think I can identify is that it never seems to remove a class. Only adds more but I've had problems attempting to test this on plunker. Any help would be appreciated. What problems can you identify with this method?

Want to make an edit: This questions is purely for my own understanding. I'm not intending to use this or re-write a tool that already exists in jQuery! Thanks for all who have submitted answers so far!

Edit: For anyone who may be interested. This isn't a perfect solution (adds spaces between classes the more you toggle). It seems to get around the false positive the original code would cause!

<!DOCTYPE html>
<html>
  <head>
    <link rel="stylesheet" href="style.css">
      <script src="https://code.jquery.com/jquery-1.10.2.js"></script>
    </head>
  <body>
    <h1 class="football">Hello Plunker!</h1>
    <script>
      function toggleElementClass(element, className) {
        var regex = new RegExp('\\b' + className + '\\b', 'g');
        if (regex.test(element.className)) {
        element.className = element.className.replace(regex, '');
      } else {
        element.className += " " + className;
      };
    };

    $("h1").click(function() {
      toggleElementClass(this, "test")
    })
    </script>
  </body>
</html>

Upvotes: 2

Views: 482

Answers (2)

spanky
spanky

Reputation: 2870

Here are some problems:

  • You'll get false positives for a partial class name match (searching for foo, will match foobar). If you fix this problem, be careful how you do it, since you must allow for more than one kind of space separator.

  • The replacement will leave extra spaces, which isn't huge, but can add up.

  • You're using a regular expression with the assumption that the class name will not contain any special regex characters. You already are getting the index, so why not use that? You'll need a loop to make sure they all get removed, but it's going to be safer.

Upvotes: 0

Rory McCrossan
Rory McCrossan

Reputation: 337570

The logic is fine for most circumstances, although will get false positives when searching for foo and a football class is available.

The specific issue with your code is with how you are attaching the click event to the h1. Currently you're setting the result of the function call to the event handler, not the function reference. This means the function is called immediately on load and the scope is not what you're expecting (it's the window instead of the h1) hence the 'undefined' error you receive.

To fix this you need to wrap the click event handler in an anonymous function:

function toggleElementClass(element, className) {
  if (element.className.indexOf(className) > -1) {
    element.className = element.className.replace(new RegExp(className, 'g'), '');
  } else {
    element.className += " " + className;
  };
};

$("h1").click(function() {
  toggleElementClass(this, "a")
})
.a {
  color: #c00;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<h1 class="a">Hello Plunker!</h1>

That being said the function is completely redundant, as you can use either jQuery's toggleClass():

$("h1").click(function() {
  $(this).toggleClass('a')
})
.a {
  color: #c00;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<h1 class="a">Hello Plunker!</h1>

Or alternatively you can use classList.toggle():

document.querySelector('h1').addEventListener('click', function() {
  this.classList.toggle('a');
});
.a {
  color: #c00;
}
<h1 class="a">Hello Plunker!</h1>

Upvotes: 3

Related Questions