3
\$\begingroup\$

I have a function that I created in Codepen. It does what I want, where the div that you select gets a blue border and changes its text. But I would like to know if there is any way to make the JavaScript that actually makes this work more compact/efficient.

Codepen

JavaScript

function toggle(el) {
  const contentReference = document.querySelectorAll('.content');
  for (div of contentReference) { div.className = 'content'; }
  el.className = 'content selected';
  const selectedContentReference = document.querySelectorAll('.selectedContent');
  for (div of selectedContentReference) {
    if (div.parentNode.className == 'content') {
      div.className = 'selectedContent contentThatShouldBeHidden';
    }
    else if (div.parentNode.className == 'content selected') {
      div.className = 'selectedContent';
    }
  }
  const notselectedContentReference = document.querySelectorAll('.notselectedContent');
  for (div of notselectedContentReference) {
    if (div.parentNode.className == 'content') {
      div.className = 'notselectedContent';
    }
    else if (div.parentNode.className == 'content selected') {
      div.className = 'notselectedContent contentThatShouldBeHidden';
    }
  }
}


const selectedContentReference = document.querySelectorAll('.selectedContent');
  for (div of selectedContentReference) {
    if (div.parentNode.className == 'content') {
      div.className = 'selectedContent contentThatShouldBeHidden';
    }
    else if (div.parentNode.className == 'content selected') {
      div.className = 'selectedContent';
    }
  }
  const notselectedContentReference = document.querySelectorAll('.notselectedContent');
  for (div of notselectedContentReference) {
    if (div.parentNode.className == 'content') {
      div.className = 'notselectedContent';
    }
    else if (div.parentNode.className == 'content selected') {
      div.className = 'notselectedContent contentThatShouldBeHidden';
    }
  }
```
\$\endgroup\$
1
  • \$\begingroup\$ The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Jun 14, 2020 at 10:32

1 Answer 1

3
\$\begingroup\$

To be honest this is very over-engineered. The same can be done with very little to no JavaScript by taking advantange of the features of CSS. But one by one.


First off the class name content is bad choice. It is very generic. It runs into the danger to be used in some other context on the same page and it doesn't describe what you are using it for. Something like selectable would be better.


Using className to identify elements is for one verbose, for the other unrelatiabe. An element, for example, that has both the classes content and selected could have a className "content selected" or "selected content" or "content selected someOtherClass", etc. Just because you set it to "content selected" there is no guarantee that it will stay that.

Instead you should use the classList property. It allows you the set, remove, toggle and check for separate classes (and more).

So, for example, the block

const selectedContentReference = document.querySelectorAll('.selectedContent');
for (div of selectedContentReference) {
  if (div.parentNode.className == 'content') {
    div.className = 'selectedContent contentThatShouldBeHidden';
  }
  else if (div.parentNode.className == 'content selected') {
    div.className = 'selectedContent';
  }
}

can become:

const selectedContentReference = document.querySelectorAll('.selectedContent');
for (div of selectedContentReference) {
  const parentIsSelected = div.parentNode.classList.contains("selected");
  div.classList.toggle("contentThatShouldBeHidden", !parentIsSelected);
}

There is more that could be said to the JavaScript, however all this class toggling is unnecessary. By using, for example, the CSS child combinator and the :not pseudo class you can show and hide the approprate texts depending on the selected class on the parent element alone.

Using the same HTML and limiting the toggle function to:

// Moving the list of elements outside the function, because the list doesn't change
const contentReference = document.querySelectorAll('.content');

function toggle(el) {
  for (div of contentReference) { div.classList.remove("selected"); }
  el.classList.add("selected");
}

and following CSS:

.content:not(.selected) > .selectedContent, 
.content.selected > .notselectedContent {
   display: none;
}

And as I mentioned at the beginning it is possible to do this without JavaScript. HTML has such a toggling feature built in: radio buttons. And CSS can be used to style elements depending on if a radio button is selected (:checked) or not, even if the radio button isn't visible.

https://jsfiddle.net/27kw8qe1/

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.