ClassList toggle works only for some elements

Question

I am an absolute beginner in this field. And what i m doing is a small todo-list page. And I am stuck in the following 'button events' part.

As in the code, I want to toggle the class of 'completed' to the ancestor div of the button clicked. However, it seems to work only for some elements. Meaning, if the nodelist of 'finishBtn' variable has an odd number length, the class list only toggles for even number indexes and vice versa.

For example, if nodelist.length = 3, then the class list only toggles for nodelist[0] and nodelist[2].

(However, for removeBtn variable, it works just fine)

Thank you very much for your time. I would really appreciate every reply of yours. Cos I m stuck in this bloody thing for hours.

    addBtn.addEventListener('click', (e)=> {
        e.preventDefault();
        if(input.value === ''){
            return;
        }

        // adding elements to the body;

        const eachTodo = document.createElement('div');
        eachTodo.classList.add('eachTodo');
        const textName = document.createElement('p');
        textName.textContent = input.value;
        const btns = document.createElement('div');
        btns.classList.add('btns');
        const finish = document.createElement('button');
        finish.classList.add('finish');
        finish.textContent = 'Finish';
        const remove = document.createElement('button');
        remove.classList.add('remove');
        remove.textContent = 'Remove';
        btns.append(finish, remove);
        eachTodo.append(textName, btns);
        plansDiv.append(eachTodo);

        input.value = '';

        //button Events
        const finishBtn = document.querySelectorAll('.finish');
        const removeBtn = document.querySelectorAll('.remove');
        finishBtn.forEach(btn => {
            btn.addEventListener('click', ()=>{
                btn.parentElement.parentElement.classList.toggle('completed');
            })
        })

        removeBtn.forEach(btn => {
            btn.addEventListener('click', ()=>{
                btn.parentElement.parentElement.remove();
            })
        })
    })

This is my CSS part

.completed p {
text-decoration: line-through;
opacity: 0.8;
}

pilchard · Accepted Answer · 2022-04-30T12:12:03.087

As it stands you're querying all buttons on each add and adding new listeners to them all resulting in duplicate listeners on each button that fire sequentially.

Elements with an even number of listeners attached will toggle the class an even number of times and return to the initial value.

"on" toggle-> "off" toggle-> "on"

Elements with an odd number of attached listeners toggle the class an odd number of times and appear correct at the end.

"on" toggle-> "off" toggle-> "on" toggle-> "off"

(It works for Remove because the first listener removes the element and subsequent Removes don't change that.)

Add listeners only once to each button

You can avoid this by simply adding the listeners directly to the newly created buttons. You already have references to each button and their parent element (eachTodo) in the script, so you can just add the listeners directly to them and reference the parent directly.

finish.addEventListener('click', () => {
  eachTodo.classList.toggle('completed');
});

remove.addEventListener('click', () => {
  eachTodo.remove();
});

const addBtn = document.getElementById('addBtn');
const plansDiv = document.getElementById('plansDiv');
const input = document.getElementById('input');

addBtn.addEventListener('click', (e) => {
  e.preventDefault();
  if (input.value === '') {
    return;
  }

  // adding elements to the body;

  const eachTodo = document.createElement('div');
  eachTodo.classList.add('eachTodo');
  const textName = document.createElement('p');
  textName.textContent = input.value;
  const btns = document.createElement('div');
  btns.classList.add('btns');
  const finish = document.createElement('button');
  finish.classList.add('finish');
  finish.textContent = 'Finish';
  const remove = document.createElement('button');
  remove.classList.add('remove');
  remove.textContent = 'Remove';
  btns.append(finish, remove);
  eachTodo.append(textName, btns);
  plansDiv.append(eachTodo);

  input.value = '';

  // Add listeners directly
  finish.addEventListener('click', () => {
    eachTodo.classList.toggle('completed');
  })

  remove.addEventListener('click', () => {
    eachTodo.remove();
  })

})

.completed p {
text-decoration: line-through;
opacity: 0.8;
}

<input type="text" id="input">
<button type="button" id="addBtn">Add</button>
<div id="plansDiv"></div>

Event delegation

A more concise solution would be to use event delegation and handle all the buttons in a single handler added to the document. Here replacing parentElement with closest('.eachTodo') to avoid the fragility of a specific ancestor depth, and checking which button was clicked using Element.matches().

document.addEventListener('click', (e) => {
  if (e.target.matches('button.finish')){
    e.target.closest('.eachTodo').classList.toggle('completed');
  }
  if (e.target.matches('button.remove')){
    e.target.closest('.eachTodo').remove();
  }
});

const addBtn = document.getElementById('addBtn');
const plansDiv = document.getElementById('plansDiv');
const input = document.getElementById('input');

document.addEventListener('click', (e) => {
  if (e.target.matches('button.finish')){
    e.target.closest('.eachTodo').classList.toggle('completed');
  }
  if (e.target.matches('button.remove')){
    e.target.closest('.eachTodo').remove();
  }
});

addBtn.addEventListener('click', (e) => {
  if (input.value === '') {
    return;
  }

  // adding elements to the body;
  const eachTodo = document.createElement('div');
  eachTodo.classList.add('eachTodo');
  const textName = document.createElement('p');
  textName.textContent = input.value;
  const btns = document.createElement('div');
  btns.classList.add('btns');
  
  const finish = document.createElement('button');
  finish.classList.add('finish');
  finish.textContent = 'Finish';
  finish.type = 'button';
  
  const remove = document.createElement('button');
  remove.classList.add('remove');
  remove.textContent = 'Remove';
  remove.type = 'button';
  
  btns.append(finish, remove);
  eachTodo.append(textName, btns);
  plansDiv.append(eachTodo);

  input.value = '';
});

.completed p {
text-decoration: line-through;
opacity: 0.8;
}

<input type="text" id="input">
<button type="button" id="addBtn">Add</button>
<div id="plansDiv"></div>

Note that you can also avoid the necessity of e.preventDefault() by specifying type="button" when you create your buttons. see: The Button element: type

ClassList toggle works only for some elements

1 Answers1

Add listeners only once to each button

Event delegation