4
\$\begingroup\$

The method toggle of Element.classList expects a parameter which names a CSS-class.

You can't assign multiple classes at once by using for example an array.

So you have to write something like ...

div.classList.toggle("hidden-element");
div.classList.toggle("border-defs");
div.classList.toggle("green-theme");

Therefore I've made an function to which a list of classes can be passed.

var toggleButton = document.querySelector('.toggle-classes');

/**
 * Adds / removes CSS-class(es) from an HTML-element.
 
 * 1. String: CSS-class or a list of CSS-classes. Within a list
 *   the single values have to be separated by a space.
 * 2. String: A CSS-selector pointing to the HTML-element upon
 *   which the class(es) shall be added / removed.

 * Returns true to indicate success.
 * Throws Error in case of failure.
*/

function toggleClasses(cssSelector, listOfClasses) {
  var element;
  var classes;
		
  if (arguments.length !== toggleClasses.length)
    throw new Error('Lack of required parameter.');

  if (typeof listOfClasses !== 'string' || listOfClasses.length === 0) 
    throw new Error('List of CSS-classes is not valid.');

  classes = listOfClasses.split(/\s/); 
 
  if (typeof cssSelector !== 'string' || cssSelector.length === 0) 
    throw new Error('Given CSS-selector is not valid.');

  element = document.querySelector(cssSelector);

  if (element === null) 
    throw new Error('No HTML-element with selector ' + cssSelector + ' found.');

  classes.forEach(function(classItem, i) {
    element.className.indexOf(classItem) === -1 
      ? element.classList.add(classItem)
      : element.classList.remove(classItem);
  });

  return true
}

// ------ USAGE EXAMPLE --------------------------------------------
var listPos = 'shadows font-special red-theme blue-theme misc-foo';

toggleButton.addEventListener('click', function() {
  try {
    toggleClasses('.demo', listPos);
  } catch (e) {
    console.error(e.message);
  }
});
.wrap {
  margin: 20px auto;
  width: 900px;
  background-color: rgba(245, 245, 245, 1.0);
  padding: 20px 30px;
}

.demo {
  width: 150px;
  height: 80px;
  line-height: 80px;
  text-align: center;
  margin: 10px 0;
}

.blue-theme {
  border: 3px solid pink;
  background-color: teal;
  color: white;
  font-weight: 800;
}

.red-theme {
  border: 3px solid teal;
  background-color: pink;
}

.shadows {
  text-shadow: 3px 3px 3px white;
  box-shadow: 3px 3px 3px grey;
}

.font-special {
  color: grey;
  font-weight: bold;
  font-family: georgia;
}

.misc-foo {
  border-radius: 12px;
  transform: skewY(5deg);
}
<div class="wrap">
  <button class="toggle-classes">Toggle classes</button>

  <div class="demo blue-theme">Demonstration</div>
</div>

I'm still not sure if I shall stay with the way of assigning the classes. One could also use an array instead of a space-separated string.

Or make it optional? If an array is assigned then that is used. If an string is assigned then it is separated.

Or would it be rather confusing to offer multiple options and better to stick with just one?

I also ask myself if my approach of throwing errors isn't a bit too drastically.

The CSS-classes are just for styling. So an application could go on with it's main purpose even if the function fails. It isn't really necessary to react so hard. Should I use something else instead? Returning, for example, a specified value?

Any hints and suggestions concerning flaws and possible improvements welcome.

\$\endgroup\$
  • \$\begingroup\$ Can you please removs comments or at least make them less....like this? It's pretty hard to read. \$\endgroup\$ – Bálint Apr 14 '16 at 17:05
  • \$\begingroup\$ @Bálint The general explanations before the function are essential for using the function as supposed. Do you consider the comments within the function as irritating? Actually I added them to provide the opposite. \$\endgroup\$ – michael.zech Apr 15 '16 at 5:59
  • \$\begingroup\$ In my opinion they are, especially on mobile devices where text soft wraps. If comments are needed, put them inline. \$\endgroup\$ – Bálint Apr 15 '16 at 6:26
  • \$\begingroup\$ It's hard to tell what your use case is based on this example, but I recommend that you avoid setting multiple classes on an element based on one cause. If you have one triggering event, then you should consider setting one class in response, and letting the CSS cascade take care of all the visual consequences of the new condition. Here's an illustration of that principle, where setting one class on an element results in drastic cascading changes. \$\endgroup\$ – 200_success Apr 15 '16 at 6:44
3
\$\begingroup\$

This code is well-done, clear and rigourous, and it works fine.
But I would not work exactly the same.

Error processing.

I agree with your doubts, like this one:

I also ask myself if my approach of throwing errors isn't a bit too drastically.

and this one:

It isn't really necessary to react so hard.

As you already noticed this kind of function, only working on style, has nothing critical about failure.
So IMO it's preferable to silently not work when it's not possible, either because the target element is not found, or the list of classes is empty.

Obviously, at the opposite, it must take care of any "technical" failure which might fire an error message, but this should not lead to something blocking.
The only "not-silent" point I think good to keep is the true-or-undefined return: this allows to know how it turned when one is concerned, while it can be ignored otherwise.

Following this point of view, I suggest to merely ensure each involved data is clean, and work only if so.
As a supplemental benefit, it ends with a reduced code:

  • first regarding the presence of the two arguments, we can merely assign default values with classes = classes || ''; and selector = selector || '';
  • then use a unique if to ensure:
    • they are not of bad type
    • they are not empty
    • the selector really targets an element

List of classes argument.

Like you suggested:

I'm still not sure if I shall stay with the way of assigning the classes. One could also use an array instead of a space-separated string.

I think it can be useful to offer a versatile way, even adding not only an array or a space-separated string but also a comma-separated string (with optional spaces).

This triple capability can be easily achieved by:

if (!Array.isArray(classes)) {
  classes = classes.replace(/(,\s*|\s+)/g, ' ').split(' ');
}

The toggle process itself.

Since it was your starting point, I'm surprised you didn't use the toggleClass() method in the final working part of your code:

element.className.indexOf(classItem) === -1 
  ? element.classList.add(classItem)
  : element.classList.remove(classItem);

It can be replaced by:

element.classList.toggleClass(classItem);

Finally here is my proposed version, following all the above remarks:

/**
 * Adds / removes CSS-class(es) from an HTML-element.
 *  
 * @param selector: a CSS selector for the involved element (string)
 *
 * @param classes: list of classes to be toggled, either:
 * - (string) comma- or space(s)-separated list of classes
 * - (array) of classes
 *
 * @return:
 * - true if could work
 * - undefined if any error occurred (bad, empty, or unsuccessfull
 *   selector; bad or empty classes)
 */

function toggleClasses(selector, classes) {
  selector = selector || '';
  classes = classes || '';
  if (
    typeof selector == 'string'
  &&
    (typeof classes == 'string' || Array.isArray(classes))
  &&
    classes.length // true if either string or array not empty
  &&
    (selector = document.querySelector(selector))
  ) {
    var classList = selector.classList;
    if (!Array.isArray(classes)) {
      classes = classes.replace(/(,\s*|\s+)/g, ' ').split(' ');
    }
    for (var className of classes) {
      classList.toggle(className);
    }
    return true;
  }
}

document.querySelector('.toggle-classes')
  .addEventListener('click', function() {
    console.log(toggleClasses(
    '.demo', 'shadows font-special red-theme blue-theme misc-foo'
    ));
});
.wrap {
  margin: 20px auto;
  width: 900px;
  background-color: rgba(245, 245, 245, 1.0);
  padding: 20px 30px;
}

.demo {
  width: 150px;
  height: 80px;
  line-height: 80px;
  text-align: center;
  margin: 10px 0;
}

.blue-theme {
  border: 3px solid pink;
  background-color: teal;
  color: white;
  font-weight: 800;
}

.red-theme {
  border: 3px solid teal;
  background-color: pink;
}

.shadows {
  text-shadow: 3px 3px 3px white;
  box-shadow: 3px 3px 3px grey;
}

.font-special {
  color: grey;
  font-weight: bold;
  font-family: georgia;
}

.misc-foo {
  border-radius: 12px;
  transform: skewY(5deg);
}
<div class="wrap">
  <button class="toggle-classes">Toggle classes</button>

  <div class="demo blue-theme">Demonstration</div>
</div>

\$\endgroup\$
  • \$\begingroup\$ "Since it was your starting point, I'm surprised you didn't use the toggleClass()" You're completely right. I often think to complicate, so that I don't see the simple option. ;)) Thanks a bunch. \$\endgroup\$ – michael.zech Apr 15 '16 at 6:14
  • \$\begingroup\$ "Live silently, die silently." \$\endgroup\$ – Bálint Apr 15 '16 at 6:28
  • \$\begingroup\$ @st88 "I often think to complicate": this is the counterpart of your strictness and accuracy, which are otherwise very welcome when writing code. I know that, it took me times (tens of years!) to first realize it, then begin to moderate :) Be quiet: you'll find the good compromise soon, without giving up your conscientiousness. Glad to help! \$\endgroup\$ – cFreed Apr 15 '16 at 8:14

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.