Change text on the click of a button, JavaScript

Question

I have a simple JavaScript expanding and collapsing text on the click of a button. What I want to do is add a "+" sign next to the button and change it to "-" when the text is collapsed and vice-versa.

This is my HTML:

<button onclick="expand('one')">First text</button><span class="plusminus">+</span>
<div class="text" id="one">
      A bunch of text here
</div>


<p><button onclick="expand('two')">Second text</button><span class="plusminus">+</span>
<div class="text" id="two">
      More text here
</div>


<p><button onclick="expand('three')">Third text</button><span class="plusminus">+</span>
<div class="text" id="three">
      And some text here
</div>

This is my CSS:

.text {
  display: none;
}

And this is JavaScript:

function expand(textId) {
    var e = document.getElementById(textId);
    var sign = document.getElementsByClassName("plusminus");
    if (e.style.display === "block") {
        e.style.display = "none";
        sign.innerText = "+";
    } else {
        e.style.display = "block";
        sign.innerText = "-";
    }
}

The expand/collapse works, but not changing the sign... What am I doing wrong?

Thanks a lot!

Several problems here. Firstly, your markup is invalid as you're not closing the `p` tags. Secondly, your selector is currently selecting ALL `plusmin` elements, not the one relative to the area being expanded/collapsed. Finally, inline DOM-zero events via HTML attributes are pretty archaic. Consider centralised event handling via [`addEventListener`](https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener) — Mitya, Feb 07 '18 at 12:57
Thanks for your comments! I think I know how to fix it, will try later. — downer, Feb 07 '18 at 13:06

score 1 · Answer 1 · answered Feb 07 '18 at 13:07

First of all, let's improve the code by converting to centralised event handling via addEventListener rather than multiple, inline event attributes muddying the HTML.

In preparation for this, we need to transfer the flag ("one", "two") etc to a data attribute to each button. So the HTML becomes:

<p>
    <button data-which="three">Third text</button>
    <span class="plusminus">+</span>
</p> <!-- <-- note missing closing P tag in your original markup -->

So in an external JS file you'd do something like:

var btns = document.querySelectorAll('button[data-which]');
[].forEach.call(btns, function(btn) {
    btn.addEventListener('click', function() {
        expand(this.getAttribute('data-which'));
    }, false);
});

Your other problem is that the expand() function currently targets ALL plusminus elements, not the one relative to the area being expanded/collapsed. Let's fix that:

var sign = document.querySelector('#'+textId).parentNode.querySelector('.plusminus');

Fiddle

score 0 · Accepted Answer · answered Feb 07 '18 at 13:07

document.getElementsByClassName returns an array. You are treating it as a single object.

Your implementation of document.getElementsByClassName is not correct, you have multiple spans with same class, you need to pick the correct one using index. keeping mind in your existing code I have done a small correction in your expand function.

function expand(textId, index) {
    var e = document.getElementById(textId);
    var sign =  document.getElementsByClassName("plusminus")[index];
    if (e.style.display === "block") {
        e.style.display = "none";
        sign.innerText = "+";
    } else {
        e.style.display = "block";
        sign.innerText = "-";
    }
}

.text {
  display: none;
}

<!DOCTYPE html>
<html>

  <head>
   
  </head>

  <body>
    <button onclick="expand('one',0)">First text</button><span class="plusminus">+</span>
<div class="text" id="one">
      A bunch of text here
</div>


<p><button onclick="expand('two',1)">Second text</button><span class="plusminus">+</span>
<div class="text" id="two">
      More text here
</div>


<p><button onclick="expand('three',2)">Third text</button><span class="plusminus">+</span>
<div class="text" id="three">
      And some text here
</div>
  </body>

</html>

score 0 · Answer 3 · edited Jun 20 '20 at 09:12

You can use the list of class classList to check if class text was applied to the element.
Use the nextElementSibling to get the minus/plus element.

function expand(button, textId) {
  var e = document.getElementById(textId);
  if (e.classList.contains('text')) {
    e.classList.remove('text');
    button.nextElementSibling.innerText = "-"
  } else {
    e.classList.add('text');
    button.nextElementSibling.innerText = "+"
  }
}

Look at this code snippet

function expand(button, textId) {
  var e = document.getElementById(textId);
  if (e.classList.contains('text')) {
    e.classList.remove('text');
    button.nextElementSibling.innerText = "-"
  } else {
    e.classList.add('text');
    button.nextElementSibling.innerText = "+"
  }
}

.text {
  display: none;
}

<button onclick="expand(this, 'one')">First text</button><span class="plusminus">+</span>
<div class="text" id="one">
  A bunch of text here
</div>
<p/>
<button onclick="expand(this, 'two')">Second text</button><span class="plusminus">+</span>
<div class="text" id="two">
  More text here
</div>
<p/>

<button onclick="expand(this, 'three')">Third text</button><span class="plusminus">+</span>
<div class="text" id="three">
  And some text here
</div>

See? now, your minus/plus element is changing.

Better approach

Bind a click event to the button using addEventListener function.
Add a target as data attribute data-target to your button, i.e data-target='one'

Look at this code snippet

document.querySelectorAll('button').forEach(function(b) {
  b.addEventListener('click', function(e) {
    expand(e.currentTarget, e.currentTarget.dataset.target);
  });
});


function expand(button, textId) {
  var e = document.getElementById(textId);
  if (e.classList.contains('text')) {
    e.classList.remove('text');
    button.nextElementSibling.innerText = "-"
  } else {
    e.classList.add('text');
    button.nextElementSibling.innerText = "+"
  }
}

.text {
  display: none;
}

<button data-target='one'>First text</button><span class="plusminus">+</span>
<div class="text" id="one">
  A bunch of text here
</div>
<p/>
<button data-target='two'>Second text</button><span class="plusminus">+</span>
<div class="text" id="two">
  More text here
</div>
<p/>

<button data-target='three'>Third text</button><span class="plusminus">+</span>
<div class="text" id="three">
  And some text here
</div>

score 0 · Answer 4 · answered Feb 07 '18 at 13:22

You can eliminate lots of mistakes and errors by following best practices:

Don’t use onclick attributes
Use event delegation
Use CSS classes to toggle styles instead of direct style changes

Also, close <p>s properly and keep in mind that <div>s can’t be inside a <p>.

Now, your most important issue was that you misunderstood what getElementsByClassName returns: an HTMLCollection, rather than a single element. innerText doesn’t mean anything on it.

document.addEventListener("click", function expand(e) {
  if (e.target.dataset.expand) {
    let textElement = document.getElementById(e.target.dataset.expand),
      expandIndicator = e.target.parentElement.getElementsByClassName("plusminus")[0];

    expandIndicator.innerText = (textElement.classList.toggle("show")
      ? "-"
      : "+");
  }
});

.text {
  display: none;
}

.show {
  display: block;
}

<p><button data-expand="one">First text</button><span class="plusminus">+</span></p>
<div id="one" class="text">
  A bunch of text here
</div>

<p><button data-expand="two">Second text</button><span class="plusminus">+</span></p>
<div id="two" class="text">
  More text here
</div>

<p><button data-expand="three">Third text</button><span class="plusminus">+</span></p>
<div id="three" class="text">
  And some text here
</div>

Instead of onclick attributes you could use data-* attributes to store the ID of the element you want to show. By event delegation (testing for e.target’s properties), you can select the correct textElement.

To find the correct expandIndicator you can go back to your parent element (<p>) and find an element with class="plusminus" from there. Remember to select the first one ([0]), since you’ll still get an HTMLCollection.

You could alternatively use expandIndicator = e.target.nextElementSibling, if the <span class="plusminus"> always comes after the button — here, the class name doesn’t event matter.

Finally, instead of testing for the style property, consider using class toggling instead. .classList.toggle conveniently returns true or false depending on whether the class has been set or not, which can directly be used in a ternary expression (… ? … : …).

Change text on the click of a button, JavaScript

4 Answers4

Look at this code snippet

Better approach

Look at this code snippet