1

I am studying the code at https://youmightnotneedjquery.com/#remove_class:

if (el.classList)
  el.classList.remove(className);
else
  el.className = el.className.replace(new RegExp('(^|\\b)' + className.split(' ').join('|') + '(\\b|$)', 'gi'), ' ');

I just wonder why it bother to use RegExp to perform the replacement? Why don't use a more simple version, like below:

if (el.classList)
  el.classList.remove(className);
else
  el.className = el.className.replace(className, ' ');

Update I think the first version has a bug. HTML class name is case sensitive, see Are class names in CSS selectors case sensitive?, so flag 'i' should not be used in the RegExp.

alancc
  • 487
  • 2
  • 24
  • 68
  • What if you want to remove the `"foo"` class name from `class="foosball foobarbaz foo foot"`? Your suggested solution would result in `" sball foobarbaz foo foot"` – Phil Oct 29 '21 at 01:14
  • `.replace` only replaces the _first_ instance of the matching string. If you want to remove ALL instances you use a regex. There is a newer method `.replaceAll`, though you'll have to check it's support: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll#browser_compatibility – Jayce444 Oct 29 '21 at 01:16
  • @Barmar that's not true, the `className` attribute is just a string, and you can give an element the same class multiple times, e.g. `class='dark footer footer'`. If you only removed the first "footer" in the string, the second one would still be there and the class style would still be applied – Jayce444 Oct 29 '21 at 01:20
  • @Jayce444 I know, I deleted my comment after testing it. I assumed it would be canonicalized. – Barmar Oct 29 '21 at 01:37

2 Answers2

3

Because you could have a class that contains the specified class as a substring.

Suppose you have an element with class="abc1 abc" and you want to remove abc. Your code will replace the first abc, so it will change to class="1 abc" instead of class="abc1".

Barmar
  • 741,623
  • 53
  • 500
  • 612
  • Thank you. What is the purpose of className.split(' ').join('|'), this seems useless if I just need: 1. Replace whole word with '(^|\\b)' and '(\\b|$)'. 2 Replace all instances with 'gi' flags. – alancc Oct 29 '21 at 01:28
  • `removeClass()` allows you to give multiple classes to remove as a space-separated list. So it's turning `"foo bar"` into the regexp `/(^|\b)foo|bar(\b|$)/gi` – Barmar Oct 29 '21 at 01:31
  • BTW, that code is buggy. `foo|bar` needs to be grouped. – Barmar Oct 29 '21 at 01:32
  • And there's no need for `^` and `$`, because `\b` matches at the ends. – Barmar Oct 29 '21 at 01:33
  • See the other answer, it explains it in greater detail. – Barmar Oct 29 '21 at 01:36
1

It doesn't look like a good approach, either in general or in that particular example.

That regular expression seems to be intended to allow strings that are a space-separated list of class names to be passed, instead of separate strings like you would do with HTMLElement.classList.remove(). If you try to pass a string containing a space to HTMLElement.classList.remove() it will throw a DOMException.

I don't think the regular expression approach you found on that website would work as it's intended to. Let's take a look of a couple of example input strings, and the resulting regular expressions:

string: 'class-a'
regex: /(^|\b)class-a(\b|$)/gi

This regular expression is pretty clear, it's looking first for either the beginning of the string or a word break, then for the string "class-a", then for either a word break or the end of the string.

Because the line of code contains className.split(' '), it looks like it's intended to handle strings containing space characters. Like this:

string: 'class-a class-b'
regex: /(^|\b)class-a|class-b(\b|$)/gi

However, because it doesn't wrap the class name section of the regular expression in brackets to create its own group, it ends up being the above. That regular expression is split down the middle by that | character, and it looks for two things to match:

  1. Either the beginning of the string or a word break, followed by "class-a".
  2. "class-b" followed by either a word break or the end of the string.

That means it would match classes it shouldn't for a classList like class-a2 thisclass-b.

There's another couple of issues as well. CSS classes are case sensitive, so it doesn't make sense to use a case insensitive regular expression. Also, the regular expression is trying to use \b to detect the beginning or end of a CSS class name, but \b will also match boundaries between letters and hyphens, so the string 'class-a-1' would match the regular expression /(^|\b)class-a(\b|$)/.

Instead, you'd do better by converting your element's classList string into an array of individual classes using split(' '), then find your class in that array of strings and remove it using something like Array.prototype.splice, then rejoin the array back into a classList string and overwrite the old one. Something like this:

if (el.classList) {
  el.classList.remove(className);
} else {
  let classes = el.className.split(' ');
  let classIndex = classes.indexOf(className);
  if (classIndex !== -1) {
    classes.splice(classIndex, 1);
  }
  el.className = classes.join(' ');
}
Mark Hanna
  • 3,206
  • 1
  • 17
  • 25
  • `^` and `$` are redundant, since word boundaries match there. – Barmar Oct 29 '21 at 01:36
  • Sure, but unlike the other issues with that approach at least those mistakes don't cause any actual issues. – Mark Hanna Oct 29 '21 at 01:37
  • Very true -- they're just more examples of the author not knowing what they're doing. – Barmar Oct 29 '21 at 01:38
  • I'll bet he originally wrote `(^|\s)` but someone told him to replace that with `\b`. And in fact, the original code was more correct because of the hyphen issue. – Barmar Oct 29 '21 at 01:39