Vladimir Kulikov #4
Conversation
| @@ -0,0 +1,20 @@ | |||
| h1, h2 { | |||
There was a problem hiding this comment.
Are you sure that all h1, h2 on your page should have the same margin? It's better to add class and create css rule for class
| padding: 20px 0; | ||
| } | ||
|
|
||
| select { |
There was a problem hiding this comment.
Same with previous. Select should have class
| <label for="task">Task</label> | ||
| <input type="text" class="add-task-text form-control" id="task" aria-describedby="taskHelp" | ||
| placeholder="What should be done?"> | ||
| <div class="invalid-feedback"> |
There was a problem hiding this comment.
If you add additional functionality, it has to work. There isn't a requirement for any validation in the task description. It's a good idea to prohibit adding an empty task. But it doesn't work. I'm able to add an empty task
| <tr> | ||
| <th scope="col">Task</th> | ||
| <th scope="col">Deadline</th> | ||
| <th scope="col"></th> |
There was a problem hiding this comment.
It's strange that the column doesn't have a name. You can call it 'Actions' or something like this
| }; | ||
|
|
||
| var run = function () { | ||
| if (!localStorage.getItem('todoList')) localStorage.setItem('todoList', "[]"); |
There was a problem hiding this comment.
It's better to write like this (even when you have only one action):
if (...) {
...
}
|
|
||
| var removeTaskBtns = document.querySelectorAll(DOMSelectors.removeTaskBtns); | ||
| removeTaskBtns.forEach(function (btn) { | ||
| btn.addEventListener('click', removeTask); |
There was a problem hiding this comment.
You can just add a listener for the delete button when you create a new task. It'll work faster
|
|
||
| var completeTaskBtns = document.querySelectorAll(DOMSelectors.completeTaskBtns); | ||
| completeTaskBtns.forEach(function (btn) { | ||
| btn.addEventListener('click', completeTask); |
|
|
||
| var filters = document.querySelectorAll(DOMSelectors.filters); | ||
| filters.forEach(function (filter) { | ||
| filter.addEventListener('change', render); |
| saveTasks(tasks); | ||
| task.value = ""; | ||
| deadline.value = ""; | ||
| render(); |
There was a problem hiding this comment.
You definitely don't need to rerender existed tasks, they haven't been changed for sure.
Here you should add a new task and listener on complete and delete.
| return task; | ||
| }); | ||
| saveTasks(updatedTasks); | ||
| render(); |
There was a problem hiding this comment.
Here also only one task has been changed
No description provided.