onClick method works on 2nd click. But it should work on the 1st click

Question

To delete it needs to be deleted when the deleteButton is clicked. But, the first time need to click 2 times. What do I have to do make it work in one click?

HTML:

    <div class="todo">
    <input id="input" type="text" maxlength="30" placeholder="text...">
    <button id="buttonAdd"><i class="fas fa-pen pencil"></i></button>
    <div id="todoMissions">   </div>
    </div>

JavaScript:

    const todoAdd = document.getElementById("buttonAdd");
    const todoDelete1 = document.getElementsByClassName("buttonDelete");

//ADD
todoAdd.addEventListener("click", () => {
  var p = document.createElement("p");
  var inputValue = document.getElementById("input").value;
  var t = document.createTextNode(inputValue);
  p.appendChild(t);
  p.setAttribute("class" , "mission");

  var html = `${inputValue} <button onclick="todoDelete()" class="buttonDelete"><i class="fas fa-trash trash"></i>`;
  p.innerHTML = html;

  if(inputValue == ""){
      alert("You must write something!");
  }
  else{
      document.getElementById("todoMissions").appendChild(p);
  }
  document.getElementById("input").value = "";
});


//DELETE
function todoDelete(){
  var close = document.getElementsByClassName("buttonDelete");
  for (var i = 0; i < close.length; i++) {
    close[i].onclick = function(){
      var div = this.parentElement;
      div.remove("mission");
    }
  }
}

Scott Marcus · Accepted Answer · 2021-02-09T17:16:30.030

Your todoDelete() function is only called after a to do item is created and it is within that function that all the items are found and given their delete event handler. So the first click gives the element you just clicked on its handler and the second click allows that handler to run, deleting the element.

Instead, the handler should be set prior to the element being created. This can be done through "event delegation", whereby you set the handler up on an ancestor of all the elements and allow the event to bubble up to that element to be handled there. Since that element will exist from the start, it's no problem to set it up on it. Then, when the event is triggered by a dynamically added element later, the event bubbles up to the ancestor, is handled there and in the handler, the actual source of the event is determined. If it is the right source, then you act appropriately. If not, do nothing. This approach not only allows for dynamically added or removed elements to immediately participate in the event handling, but it results in just a single event handler registration, rather than one for each dynamically added element, which is more efficient and easier to keep track of.

Additionally:

Don't use inline HTML event attributes like onXyz in the first place (here's even more on that).
Don't use .getElementsByClassName(), instead use .querySelectorAll() when a node list/collection is desired.

So here's your code again, using this approach (see additional comments inline):

const todoAdd = document.getElementById("buttonAdd");
const output = document.getElementById("todoMissions");

// When you are going to need to use the same element
// over and over, just get the reference just once and
// keep using it.
const input = document.getElementById("input");

// Set up the delete handler on an ancestor of all the potential items
document.querySelector(".todo").addEventListener("click", todoDelete);
    
//ADD
todoAdd.addEventListener("click", () => {
  // Best to test this first and then only do the work if there is a value.
  if(input.value == ""){
      alert("You must write something!");
  }else{
  
    var p = document.createElement("p");
    // Use the .classList API when setting/removing/toggling
    // classes instead of .setAttribute as the API is more straight-forward
    p.classList.add("mission");
  
    // No need to create a text node and then inject that into your string
    // just inject the string value right into the string.
    p.innerHTML = `${input.value} <button class="buttonDelete"><i class="fas fa-trash trash">X</i>`;  
    output.appendChild(p);
  }
  
  input.value = "";
});
    
    
//DELETE
function todoDelete(event){
  // Look at the event argument and see if the event
  // was triggered by a todo item delete button or the
  // <i> element within one.
  if(event.target.classList.contains("buttonDelete") 
    || event.target.classList.contains("trash")){
    event.target.closest("p").remove();  // Remove the nearest ancestor p
  }
}

<div class="todo">
    <input id="input" type="text" maxlength="30" placeholder="text...">
    <button id="buttonAdd"><i class="fas fa-pen pencil">Add</i></button>
    <div id="todoMissions"></div>
</div>

score 1 · Answer 2 · answered Feb 09 '21 at 17:13

I hope this helps as you already got the answer. So technically the first click you have will only create the event to delete your element, that's why it only works on your second click. So I might just put a (this) as a parameter of your todoDelete

 var html = `${inputValue} <button onclick="todoDelete(this)" class="buttonDelete">DELETE<i class="fas fa-trash trash"></i>`;

p.innerHTML = HTML;

and Changing your delete Function as this:

function todoDelete(e){
   e.parentNode.remove();
}

const todoAdd = document.getElementById("buttonAdd");
const todoDelete1 = document.getElementsByClassName("buttonDelete");

//ADD
todoAdd.addEventListener("click", () => {
  var p = document.createElement("p");
  var inputValue = document.getElementById("input").value;
  var t = document.createTextNode(inputValue);
  p.appendChild(t);
  p.setAttribute("class" , "mission");
  //by adding (this) as your parameter will help tell the function what element are you on it
  var html = `${inputValue} <button onclick="todoDelete(this)" class="buttonDelete">DELETE<i class="fas fa-trash trash"></i>`;
  p.innerHTML = html;

  if(inputValue == ""){
      alert("You must write something!");
  }
  else{
      document.getElementById("todoMissions").appendChild(p);
  }
  document.getElementById("input").value = "";
});


//DELETE
function todoDelete(e){
  // just removing the parent node of the element
  e.parentNode.remove();
}

<div class="todo">
    <input id="input" type="text" maxlength="30" placeholder="text...">
    <button id="buttonAdd">ADD<i class="fas fa-pen pencil"></i></button>
    <div id="todoMissions"></div>
</div>

onClick method works on 2nd click. But it should work on the 1st click

2 Answers2