Ex. 1 - Rotem Ben David #14
Conversation
| </div> | ||
| <div class="bottom"> | ||
| <span class="howMany"></span> | ||
| <button type="button" class="sort" style="display:none">sort</button> |
There was a problem hiding this comment.
Using a class instead of an inline style attribute is always better. Only in specific cases, using an inline style is preferred.
| @@ -0,0 +1,89 @@ | |||
| let input = document.querySelector(".newTask input"); | |||
There was a problem hiding this comment.
It could also be a constant.
Moreover, you could have used a more straightforward name. input is too generic and doesn't say much about the element it points at.
| @@ -0,0 +1,89 @@ | |||
| let input = document.querySelector(".newTask input"); | |||
| const add = document.querySelector(".newTask button"); | |||
There was a problem hiding this comment.
Again, you could have used a more specific name
| //show the tasks | ||
| function showTasks(newTask) { //adds new task to tasks array and adds it to html | ||
| let newLiTag = ''; | ||
| if (newTask != undefined) { |
There was a problem hiding this comment.
Lines 38-40 should have been part of addNewTask
This is the code that adds the task to the tasks array.
showTasks should be in charge of showing the tasks. No more logic should be in it.
| tasks.push(newTask); | ||
| } | ||
| tasks.forEach((element, index) => { | ||
| newLiTag += '<li onClick="taskAlert('+index+')">'+element+'<span onClick="deleteTask('+index+');event.stopPropagation();">🗑</span></li>'; |
There was a problem hiding this comment.
You called the variable newLiTag and eventually, it holds multiple li tags, each for every task in the tasks array.
Besides it, it is not clear what 🗑 means. Make sure you add explanations when you code is not 100% straightforward.
| else { //delete clear all button when there aren't tasks | ||
| showClearAll.style = 'display: none'; | ||
| showSort.style = 'display: none'; | ||
| howManyTasks.innerHTML = defultMassage; |
| const tasksView = document.querySelector(".tasks"); | ||
| const tasks = []; | ||
| const howManyTasks = document.querySelector(".bottom .howMany"); | ||
| const showClearAll = document.querySelector(".bottom .clear"); |
There was a problem hiding this comment.
clearAllButtom would be a better name
| const tasks = []; | ||
| const howManyTasks = document.querySelector(".bottom .howMany"); | ||
| const showClearAll = document.querySelector(".bottom .clear"); | ||
| const showSort = document.querySelector(".bottom .sort"); |
There was a problem hiding this comment.
sortButton would be a better name
|
|
||
| //sort | ||
| showSort.addEventListener('click', () => { //recognize click on 'sort' button | ||
| tasks.sort(); |
There was a problem hiding this comment.
Sort without a function is usually not recommended as it is unclear how the array is sorted.
| newSpan = '<span>You have '+tasks.length+' pending tasks</span>' | ||
| howManyTasks.innerHTML = newSpan; | ||
| if (tasks.length === 1) { //display clear all button only when there are tasks | ||
| showClearAll.style = 'display: block'; |
There was a problem hiding this comment.
@or-nuri-monday can you please explain what do you mean? (I fixed all other commants)
No description provided.