storeObjectcannot be both a type and a function. Remove thetype storeObject...statementnew Dateneeds parentheses, e.g.new Date()storeObjectdoes not need the variableitem, the object can be inlined, e.g.todos.push({ id: uuidv4(), todo, completed: false, createdDate: new Date(), })Don't use type assertions (e.g.
... as SomeType), use type guards and narrowing. For example, InaddTaskBtnevent listener, use:if (e.target instanceof HTMLElement) { if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); } }Alternatively, check at the start of the method that you have the prerequsites and bail as appropriate:
if(input.value === "" || !(e.target instanceof HTMLElement)) { return } if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); } if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); }Use strict equality
===instead of==or type coercion can lead to unexpected results.Don't use ternary expressions with side effects, use a full if/then/else expression.
Use optional chaining operator (
?.) to avoid needing to check for null, or type assertions to override the possibility of null. If your DOM does not match the expected, you should probably bail in the event that DOM queries return NULL. Alternatively, put the DOM setup into the code, and only require a container element to be passed to the setup method.updateTodo: todos.map's callback should return a value (e.g.return item)You're creating a lot of work for yourself to add an event listener to the document that then has to disambiguate which element is firing the event. Instead, add event handlers to the specific element they serve.
Instead of inlining the event handler, take advantage of the fact that
thisis the event target (you'll need to use a fat function rather than an arrow function). e.g.function editBtnHandler(this: HTMLButtonElement, ev: Event) { // this === ev.target // it's also correctly typed, so we don't need type assertions or null checks } editBtn.addEventListener('click', editBtnHandler)Wrap the whole shebang in a class. Pass to the constructor either a container element, or validated list, button and input elements so that you can lose the type assertions and optional chaining.
class TodoApp { private todos: Todo[] = [] constructor(container: HTMLElement) { const ul = document.createElement(...) //... container.appendChild(ul) //... } renderTodos() { this.todos.forEach(todo => { //... }) } } const container = document.querySelector('#todoApp') const todoApp = new TodoApp(container)Reconsider how you do the DOM traversal to get to elements containing id and label text. If you are adding a dedicated handler you could bind those values when the handler is created, so you have them to hand when handling the event.
Rather than redefining the functionality of the button/input element depending on add/update, you could use CSS classes to show/hide elements dedicated to those tasks (or update the DOM to add/remove). This way you lower the risk of keeping track of which state any element is in, and the code will read more clearly.
storeObjectcannot be both a type and a function. Remove thetype storeObject...statementnew Dateneeds parentheses, e.g.new Date()storeObjectdoes not need the variableitem, the object can be inlined, e.g.todos.push({ id: uuidv4(), todo, completed: false, createdDate: new Date(), })Don't use type assertions (e.g.
... as SomeType), use type guards and narrowing. For example, InaddTaskBtnevent listener, use:if (e.target instanceof HTMLElement) { if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); } }Alternatively, check at the start of the method that you have the prerequsites and bail as appropriate:
if(input.value === "" || !(e.target instanceof HTMLElement)) { return } if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); } if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); }Use strict equality
===instead of==or type coercion can lead to unexpected results.Don't use ternary expressions with side effects, use a full if/then/else expression.
Use optional chaining operator (
?.) to avoid needing to check for null, or type assertions to override the possibility of null. If your DOM does not match the expected, you should probably bail in the event that DOM queries return NULL. Alternatively, put the DOM setup into the code, and only require a container element to be passed to the setup method.updateTodo: todos.map's callback should return a value (e.g.return item)You're creating a lot of work for yourself to add an event listener to the document that then has to disambiguate which element is firing the event. Instead, add event handlers to the specific element they serve.
Instead of inlining the event handler, take advantage of the fact that
thisis the event target (you'll need to use a fat function rather than an arrow function).Wrap the whole shebang in a class. Pass to the constructor either a container element, or validated list, button and input elements so that you can lose the type assertions and optional chaining.
Reconsider how you do the DOM traversal to get to elements containing id and label text. If you are adding a dedicated handler you could bind those values when the handler is created, so you have them to hand when handling the event.
Rather than redefining the functionality of the button/input element depending on add/update, you could use CSS classes to show/hide elements dedicated to those tasks (or update the DOM to add/remove). This way you lower the risk of keeping track of which state any element is in, and the code will read more clearly.
storeObjectcannot be both a type and a function. Remove thetype storeObject...statementnew Dateneeds parentheses, e.g.new Date()storeObjectdoes not need the variableitem, the object can be inlined, e.g.todos.push({ id: uuidv4(), todo, completed: false, createdDate: new Date(), })Don't use type assertions (e.g.
... as SomeType), use type guards and narrowing. For example, InaddTaskBtnevent listener, use:if (e.target instanceof HTMLElement) { if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); } }Alternatively, check at the start of the method that you have the prerequsites and bail as appropriate:
if(input.value === "" || !(e.target instanceof HTMLElement)) { return } if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); } if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); }Use strict equality
===instead of==or type coercion can lead to unexpected results.Don't use ternary expressions with side effects, use a full if/then/else expression.
Use optional chaining operator (
?.) to avoid needing to check for null, or type assertions to override the possibility of null. If your DOM does not match the expected, you should probably bail in the event that DOM queries return NULL. Alternatively, put the DOM setup into the code, and only require a container element to be passed to the setup method.updateTodo: todos.map's callback should return a value (e.g.return item)You're creating a lot of work for yourself to add an event listener to the document that then has to disambiguate which element is firing the event. Instead, add event handlers to the specific element they serve.
Instead of inlining the event handler, take advantage of the fact that
thisis the event target (you'll need to use a fat function rather than an arrow function). e.g.function editBtnHandler(this: HTMLButtonElement, ev: Event) { // this === ev.target // it's also correctly typed, so we don't need type assertions or null checks } editBtn.addEventListener('click', editBtnHandler)Wrap the whole shebang in a class. Pass to the constructor either a container element, or validated list, button and input elements so that you can lose the type assertions and optional chaining.
class TodoApp { private todos: Todo[] = [] constructor(container: HTMLElement) { const ul = document.createElement(...) //... container.appendChild(ul) //... } renderTodos() { this.todos.forEach(todo => { //... }) } } const container = document.querySelector('#todoApp') const todoApp = new TodoApp(container)Reconsider how you do the DOM traversal to get to elements containing id and label text. If you are adding a dedicated handler you could bind those values when the handler is created, so you have them to hand when handling the event.
Rather than redefining the functionality of the button/input element depending on add/update, you could use CSS classes to show/hide elements dedicated to those tasks (or update the DOM to add/remove). This way you lower the risk of keeping track of which state any element is in, and the code will read more clearly.
storeObjectcannot be both a type and a function. Remove thetype storeObject...statementnew Dateneeds parentheses, e.g.new Date()storeObjectdoes not need the variableitem, the object can be inlined, e.g.todos.push({ id: uuidv4(), todo, completed: false, createdDate: new Date(), })Don't use type assertions (e.g.
... as SomeType), use type guards and narrowing. For example, InaddTaskBtnevent listener, use:if (e.target instanceof HTMLElement) { if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); } }Alternatively, check at the start of the method that you have the prerequsites and bail as appropriate:
if(input.value === "" || !(e.target instanceof HTMLElement)) { return } if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); } if (e.target.textContent === "Update") { updateTodo(id) } else { storeObject(input.value); }Use strict equality
===instead of==or type coercion can lead to unexpected results.Don't use ternary expressions with side effects, use a full if/then/else expression.
Use optional chaining operator (
?.) to avoid needing to check for null, or type assertions to override the possibility of null. If your DOM does not match the expected, you should probably bail in the event that DOM queries return NULL. Alternatively, put the DOM setup into the code, and only require a container element to be passed to the setup method.updateTodo: todos.map's callback should return a value (e.g.return item)You're creating a lot of work for yourself to add an event listener to the document that then has to disambiguate which element is firing the event. Instead, add event handlers to the specific element they serve.
Instead of inlining the event handler, take advantage of the fact that
thisis the event target (you'll need to use a fat function rather than an arrow function).Wrap the whole shebang in a class. Pass to the constructor either a container element, or validated list, button and input elements so that you can lose the type assertions and optional chaining.
Reconsider how you do the DOM traversal to get to elements containing id and label text. If you are adding a dedicated handler you could bind those values when the handler is created, so you have them to hand when handling the event.
Rather than redefining the functionality of the button/input element depending on add/update, you could use CSS classes to show/hide elements dedicated to those tasks (or update the DOM to add/remove). This way you lower the risk of keeping track of which state any element is in, and the code will read more clearly.
There is a lot there for you to do, and rather than just providing a rewrite with all of those suggestions, its probably better that you work through them and see how they apply. Come back when you have a revision, perhaps as a new post. Feel free to ask for clarifications if not sure.