Why is the done button not working properly?-CodePudding

Apparently I'm trying to create a todo list where I can ofcourse add and remove the tasks. Adding tasks works fine; however clicking on the Done Button works but doesn't do what I want it to do. Basically I have a Logical Error, but I don't know what to do to fix it.

The Code

<!DOCTYPE html>
<html lang="en">

<head>
  <title>Document</title>
</head>

<body>
  <h1>To-Do List</h1>
  <form id="todoForm">
    <input id="todoInput" />
    <button type="button" onclick="todoList()">New</button>
    <button type="button" onclick="">Retrieve</button>
  </form>
  <ol id="todoList"></ol>
  <script>
    var todos = []; //Problem is from here
    var removed = [];

    function todoList() {
      var item = document.getElementById("todoInput").value;
      todos.push(item);

      var text = document.createTextNode(item);
      var newItem = document.createElement("li");

      newItem.innerHTML = item   ' <button id="Done">Done</button>';
      document.getElementById("todoList").appendChild(newItem);

      const donebtn = document.getElementById("Done");
      donebtn.addEventListener("click", function() {
        removetodo(newItem, item)
      });
    }


    function removetodo(item, tasktext) {
      const tasklist = document.getElementById("todoList");
      tasklist.removeChild(item);
      removed.push(tasktext);
    }
  </script>
</body>

</html>

Thing is, I tried finding solutions to it on Google and other places; yet, I still didnt know how to fix it. I dont want to just change the whole code so it could work. I specifically wanted it in the way I wrote it in

CodePudding user response：

newItem.innerHTML = item ' Done'; I changed this line the problem was that you are assigning the same id's to all done so I used a count variable which is at the start 0 when you run function if will 0 like done0 when the function run it will increase count will increase it next time it will be done1 so your code will work correctly

<!DOCTYPE html>
<html lang="en">
  <head>
    <title>Document</title>
  </head>
  <body>
    <h1>To-Do List</h1>
    <form id="todoForm">
      <input id="todoInput" />
      <button type="button" onclick="todoList()">New</button>
      <button type="button" onclick="">Retrieve</button>
    </form>
    <ol id="todoList"></ol>
    <script> 
      var todos = []; //Problem is from here
      var removed = [];
      
      let count = 0;
      function todoList() { 
        var item = document.getElementById("todoInput").value;
        todos.push(item);

        var text = document.createTextNode(item);
        var newItem = document.createElement("li");

        newItem.innerHTML = item   ' <button id="Done' count '">Done</button>';
        document.getElementById("todoList").appendChild(newItem);

        const donebtn = document.getElementById("Done" count);
        donebtn.addEventListener("click", function(){
          removetodo(newItem, item)
        });
        count  ;
      }


      function removetodo(item, tasktext) {
          const tasklist = document.getElementById("todoList");
          tasklist.removeChild(item);
          removed.push(tasktext);
        }
    </script>
  </body>
</html>

one more suggestion

newItem.innerHTML = item   ' <button id="Done' count '">Done</button>';
        document.getElementById("todoList").appendChild(newItem);

        const donebtn = document.getElementById("Done" count);
        donebtn.addEventListener("click", function(){

here in your code const donebtn = document.getElementById("Done" count); you don't need this line just donebtn.addEventListener("click", function(){ here insted of donebtn you can use newItem.addEventListener and then append it document.getElementById("todoList").appendChild(newItem); at the last use this.

 newItem.innerHTML = item   ' <button id="Done' count '">Done</button>';
        newItem.addEventListener("click", function(){}
document.getElementById("todoList").appendChild(newItem);

like this

CodePudding user response：

This code will only run when your function is called.

const donebtn = document.getElementById("Done");
      donebtn.addEventListener("click", function() {
        removetodo(newItem, item)
      });

you should put it outside the functions to attach the listener.

CodePudding user response：

The first issue with the code is that when you remove the task from the list, it's not actually removing it from the todos array. To fix this, you can add the following line after removing the task from the list:

todos.splice(todos.indexOf(tasktext), 1);

The second issue is that you are using the same id for all the "Done" <button> elements, in the HTML markup, IDs should be unique. So when you use document.getElementById("Done"), it always returns the first element with that id.

To fix this issue, you can use a class instead of an id and query for all elements with that class and attach an event listener to each button individually.

Updated code:

<!DOCTYPE html>
<html lang="en">

<head>
  <title>Document</title>
</head>

<body>
  <h1>To-Do List</h1>
  <form id="todoForm">
    <input id="todoInput" />
    <button type="button" onclick="todoList()">New</button>
    <button type="button" onclick="">Retrieve</button>
  </form>
  <ol id="todoList"></ol>
  <script>
    var todos = [];
var removed = [];

function todoList() {
  let item = document.getElementById("todoInput").value;
  todos.push(item);

  let text = document.createTextNode(item);
  let newItem = document.createElement("li");

  newItem.innerHTML = item   ' <button >Done</button>';
  document.getElementById("todoList").appendChild(newItem);

  const donebtn = newItem.getElementsByClassName("doneBtn")[0];
  donebtn.addEventListener("click", function() {
    removetodo(newItem, item);
  });
}

function removetodo(item, tasktext) {
  const tasklist = document.getElementById("todoList");
  tasklist.removeChild(item);
  removed.push(tasktext);
  todos.splice(todos.indexOf(tasktext), 1);
}




  </script>
</body>

</html>

CodePudding user response：

Each time a new task is added, all the "Done" buttons have the same id, which is not allowed in HTML as id values must be unique within a page. This means that only the first "Done" button will respond to the click event, and all the others will be ignored.

One way you can try is to store the task text in a data attribute of the "Done" button, and use it in the removetodo function to identify the task to remove like so ...

<!DOCTYPE html>
<html lang="en">

<head>
  <title>Document</title>
</head>

<body>
  <h1>To-Do List</h1>
<form id="todoForm">
<input id="todoInput" />
<button type="button" onclick="todoList()">New</button>
<button type="button" onclick="">Retrieve</button>
</form>
<ol id="todoList"></ol>
<script>
var todos = []; 
var removed = [];

function todoList() {
  var item = document.getElementById("todoInput").value;
  todos.push(item);

  var text = document.createTextNode(item);
  var newItem = document.createElement("li");

  newItem.innerHTML = item   ' <button >Done</button>';
  document.getElementById("todoList").appendChild(newItem);

  const donebtn = newItem.getElementsByClassName("Done")[0];
  donebtn.addEventListener("click", function() {
    removetodo(newItem, item)
  });
  donebtn.setAttribute("data-task", item);
}


function removetodo(item, tasktext) {
  const tasklist = document.getElementById("todoList");
  tasklist.removeChild(item);
  removed.push(tasktext);
}
 </script>
</body>