Hey
So i am making my to-do app and i have encountered this weird problem where my removeToDo function targets the ID of an item in an array like this:
clickedItemID + amount of times i clicked on an list item(any in the array)
Example
If i click on item with id 1, it deletes fine. Then i click item with id 3, deletes item with id 4 (adds 1 to id because of 1 xclick), then let’s say i click item with id 5 and it deletes item with ID 7 (as of 2 x click ?) I may be wrong but that’s how it looks like … here’s some code :
const clear = document.querySelector("#clear");
const date = document.getElementById("date");
const list = document.getElementById("list");
const input = document.getElementById("itemInput");
const check = "fa-check-circle";
const uncheck = "fa-circle";
const lineThrough = "lineThrough";
let id;
//get the item from the local storage
let data = localStorage.getItem('TODO');
//check if data is not empty
if(data) {
LIST = JSON.parse(data)
id = LIST.length; // set the list id to the last in the array
loadList(LIST); // load all the items in the array to the UI
} else {
//if data is empty
LIST = [];
id = 0;
}
function loadList(array) {
array.forEach(item => {
addToDo(item.name, item.id, item.done, item.trash);
})
}
function addToDo(toDo, id, done, trash) {
// if trash is true do not execute the code below
if (trash) {return ;}
const DONE = done ? check : uncheck;
const LINE = done ? lineThrough : "";
const text =`
<li class="item">
<i class="far ${DONE}" id='${id}'></i>
<div class="description ${LINE} wrap">${toDo}</div>
<i class="fas fa-trash-alt" id='${id}'></i>
</li>`;
const position = "beforeend";
list.insertAdjacentHTML(position, text);
}
// remove to-do
function removeToDo(element) {
element.parentNode.parentNode.removeChild(element.parentNode);
LIST[element.id].trash = true;
LIST.splice(element.id, 1); // <----- I think that is the problem ?
//localStorage.removeItem(addToDo.id);
console.log(LIST);
}
// click listener for job complete and job delete
list.addEventListener('click', e => {
const element = e.target;
if(e.target.className == "fas fa-trash-alt" ){
removeToDo(element);
}else if(e.target.className == "far fa-circle") {
jobComplete(element);
}else if(e.target.className == "far fa-check-circle"){
jobComplete(element);
}
}
)
//add a task with "enter" key
document.addEventListener("keyup", (event) => {
if(event.keyCode == 13){
const toDo = input.value;
if(toDo) {
addToDo(toDo, id, false, false);
LIST.push(
{
name: toDo,
id: id,
done: false,
trash: false
}
);
localStorage.setItem('TODO', JSON.stringify(LIST));
id++;
input.value = '';
}
}
})
The use of the ids for the first splice makes sense, as they are the same as the indices, but the minute you alter the array, the indices and ids are no longer the same.
Loop through the array, and splice if element has id id.
Hope this helps.
Why do you think that is stacking event listeners? Is that code snippet inside of a loop? What you might want to do, is, before splicing the element, use removeEventListener on the element.
I think it’s stacking event listeners because even if i get the index of id it was still deleting wrong id further within the app. Like if i removed one item, it counts as firing event listener one time, so if i would try to remove three items, it would fire it three times in one click after that, resulting in index +2 ? It’s a bit of a speculation but i will try to add removeEventListener in a sec to see if it works.
EDIT
So removeEventListener doesn’t do much, i didn’t observe any change…
But i have changed the code earlier on a bit, am i getting any closer to solving this ?
// remove to-do
function removeToDo(element) {
let newList = [...LIST]
element.parentNode.parentNode.removeChild(element.parentNode);
let children = element.parentNode.childNodes;
for (i = 0; i < children.length; i++) {
if (element == children[i]) break;
}
newList[i].trash = true;
newList.splice(i, 1);
LIST = newList;
console.log(LIST);
}
It was suppose to iterate through the array to get index of the clicked item ? I am guessing it’s wrong Sorry i have spent most of the day trying to figure it out and i guess at the end i started adding stuff i found on google that looked like it may work it returns correct id values of a clicked item though
No need to apologise. I just ask, because the for loop loops through the array, until the element is the same as the child. It is just an inefficient way to do indexOf(). I suggest you research the use of indexOf.
Ahh fair enough, i tried indexOf as well but probably haven’t tried it very well. Anyway the problem still persists even if i am getting the right values when i click on the item, anything else i could try ?
EDIT: Ok i made indexOf to work properly, its getting right id values, but problem with the wrong items in the array getting removed persists
Thank your, for doing that. The functionality works as I would expect…
I can see that the alert logs -1 for any items deleted, if they were created after deleting a previous item. But, the correct item still gets deleted.
That is weird, i was getting that -1 but that was AFTER i deleted few items (and sorry i forgot to actually mention that), and that’s how it works in Studio code right now. Just checked the code and everything seems exactly the same ?
EDIT: Ok hang on it doesn’t now O_o
EDIT2: And now it does again, what the hell…
EDIT3… : Ok so anything created after deleting any item, gets the index-1. If you do “clear all” it deletes all items in the array and clears the storage, so could it be that the local storage is cause of the index problem ?
Just did a small test - had numbers 1 - 8 with ID’s 0 - 7.
Clicked to delete item with number 4 (id = 3), deleted it right from the array and console logged id number 3, which was right. Then clicked to delete item with number 5 (id = 4), deleted number 6 instead with id = 5, but the console logged id 4 ?
Instead of having a top-level id variable that you are passing around and incrementing, why not generate a unique id using something like uuid and filter the array using that id.
LIST = LIST.filter(todo => todo.id !== event.target.id);
You are also not updating localStorage in removeToDo.
@lasjorg Well i am a complete beginner, that uuid thing seems a bit complicated ? I am still learning basic javascript so i don’t want to confuse my brain with some libraries …
As for updating localStorage, that was the plan when i sort out problem with id’s as i need to learn how to do that first, but might be worth doing it now …
It’s just a function you call that gives you a guaranteed unique string, you call it uuidv4() and it returns a string like this “71b05b68-7e94-40fd-bcfe-97c925cd7b48”.
Using it is in fact much less complicated then what you are doing now. Each todos id will be guaranteed to have a fixed unique id that you can use in an array filter.
I didn’t really test this code much but just to show what I mean.
@lasjorg You have fixed all my problems there, good Sir! It is much simpler and efficient, it doesn’t look like my Visual studio code recognizes the uuidv4() though, do i need to install something ? Also could i actually achieve what i wanted to with the way i was doing this ?
I’m sure you might. But there is really no reason to have a “floating dynamic value” (i.e. a value being passed around and being changed) that you need to keep track of when all you need is just a fixed unique value that never changes.
Also, filtering an array based on a unique id is a very common way of doing this. You will see it done like this everywhere in other applications.
Sorry i have spent most of the day trying to figure it out and i guess at the end i started adding stuff i found on google that looked like it may work 
it returns correct id values of a clicked item though