Conversation
HW8(MVC)/scripts/View.js
Outdated
|
|
||
| class View { | ||
| constructor() { | ||
| this.toDoContainer = document.querySelector('.to-do'); |
There was a problem hiding this comment.
it is better to use id if you are processing only one element.
it is also worth considering error handling if the element is not found
…wing an error if not.
| const h1 = document.createElement('h1'); | ||
| h1.innerText = 'THINGS TO DO ✌️'; | ||
| document.body.prepend(h1); | ||
|
|
||
| this.showButton = document.createElement('button'); | ||
| this.showButton.classList.add('to-do__button', 'to-do__button--show'); | ||
| this.showButton.title = 'Show all tasks'; | ||
| this.showButton.innerText = 'SHOW'; | ||
| this.toDoContainer.appendChild(this.showButton); | ||
|
|
||
| this.input = document.createElement('input'); | ||
| this.input.classList.add('to-do__input'); | ||
| this.input.placeholder = 'Add a new task 👈'; | ||
| this.toDoContainer.appendChild(this.input); | ||
|
|
||
| this.addButton = document.createElement('button'); | ||
| this.addButton.classList.add('to-do__button', 'to-do__button--add'); | ||
| this.addButton.title = 'Add a new task'; | ||
| this.addButton.innerText = 'ADD'; | ||
| this.toDoContainer.appendChild(this.addButton); | ||
|
|
||
| this.tasksContainer = document.createElement('div'); | ||
| this.tasksContainer.classList.add('tasks'); | ||
| this.toDoContainer.appendChild(this.tasksContainer); | ||
|
|
||
| this.tasksList = document.createElement('ul'); | ||
| this.tasksList.classList.add('tasks__list'); | ||
| this.tasksContainer.appendChild(this.tasksList); |
There was a problem hiding this comment.
it is not the responsibility of the constructor. it should be a separate method
all class names / ids / texts are best used from constants
HW8(MVC)/scripts/View.js
Outdated
| } | ||
|
|
||
| renderTasks(tasks) { | ||
| if (tasks.length === 0 && $C.isFiltered() === 'false') { |
There was a problem hiding this comment.
Why would you take a controller as an unobvious dependency?
the method can only work with input parameters
HW8(MVC)/scripts/View.js
Outdated
| this.toggleShowButton(); | ||
| this.showButton.style.display = 'none'; | ||
| this.tasksList.style.display = 'none'; | ||
| $C.tasksListIsHidden = true; |
There was a problem hiding this comment.
can we not use the controller as a dependency?
HW8(MVC)/scripts/View.js
Outdated
| this.tasksList = document.querySelector('.tasks__list'); | ||
| this.showButton = document.querySelector('.to-do__button--show'); | ||
|
|
||
| this.tasksList.style.display = 'inline-block'; | ||
| this.showButton.style.display = 'inline-block'; |
There was a problem hiding this comment.
for clarity of use, it is better to take keys from the private properties of the class
There was a problem hiding this comment.
Not quite understood. Let`s discuss this tomorrow during the meeting.
HW8(MVC)/scripts/View.js
Outdated
| if (this.tasksList.style.height >= '25vh') { | ||
| this.tasksList.style.height = '0px'; | ||
| this.tasksList.style.overflow = 'hidden'; | ||
| this.tasksList.style.padding = '0px'; | ||
| this.showButton.innerHTML = 'SHOW'; | ||
| this.showButton.style.background = '#87ff93'; | ||
| } else { | ||
| this.tasksList.style.height = '25vh'; | ||
| this.tasksList.style.overflowY = 'scroll'; | ||
| this.tasksList.style.padding = '15px'; | ||
| this.showButton.innerHTML = 'CLOSE'; | ||
| this.showButton.style.background = '#ff8f87'; |
There was a problem hiding this comment.
to smear css on js on such a scale is bad.
can we just change the class on the wrapper? to leave css in css
HW8(MVC)/scripts/View.js
Outdated
| <li class="tasks__list-item"> | ||
| <div class="tasks__title tasks__title--last">CURRENT TASKS</div> | ||
| </li>`; |
There was a problem hiding this comment.
Do we really need to create this html by code?
HW8(MVC)/scripts/View.js
Outdated
| this.createTasksListItems(tasks); | ||
|
|
||
| let filterButtonText; | ||
| if ($C.isFiltered() === 'true') { |
There was a problem hiding this comment.
can we solve this without importing the controller?
HW8(MVC)/scripts/Model.js
Outdated
| class Model { | ||
| constructor() {} | ||
|
|
||
| addTask(title) { |
There was a problem hiding this comment.
add what task? where? can we write a better name?
| } | ||
|
|
||
| getTasksList() { | ||
| const tasks = JSON.parse(localStorage.getItem('savedTasks')) ?? { list: [] }; |
There was a problem hiding this comment.
'savedTasks' - better save in private property
There was a problem hiding this comment.
Same as above. How exactly should I implement "private"?
| const currentTasks = this.getTasksList(); | ||
|
|
||
| currentTasks.push({ | ||
| 'title': title, | ||
| 'isCompleted': false | ||
| }); |
There was a problem hiding this comment.
smearing logic. why can't all this happen in the setTasksList method?
There was a problem hiding this comment.
- Function: Adding a new task. Input - title (string)
- Function: Saving current tasks list - tasksList(array). For example, after deleting tasks.
Improved function names for a better visible difference.
HW8(MVC)/scripts/Controller.js
Outdated
| this.invalidInputTimer = null; | ||
| this.tasksListIsHidden = true; |
There was a problem hiding this comment.
it is better to describe properties explicitly.
HW8(MVC)/scripts/Controller.js
Outdated
| } | ||
| } | ||
|
|
||
| handleClick(e) { |
There was a problem hiding this comment.
handle what Click? pls, use more descriptive names.
HW8(MVC)/scripts/Controller.js
Outdated
| $V.addButton.style.backgroundColor = '#87ff93'; | ||
| } else { | ||
| $V.addButton.style.backgroundColor = '#fff'; |
There was a problem hiding this comment.
if possible, it is better to leave the css in the css file
HW8(MVC)/scripts/Controller.js
Outdated
| let isDone = tasks[index].isCompleted; | ||
|
|
||
| if (isDone) { | ||
| isDone = false; | ||
| } else { | ||
| isDone = true; | ||
| } | ||
| tasks[index].isCompleted = isDone; |
There was a problem hiding this comment.
tasks[index].isCompleted = !tasks[index].isCompleted;
HW8(MVC)/scripts/Controller.js
Outdated
| const tasks = $M.getTasksList(); | ||
| tasks.splice(index, 1); | ||
|
|
||
| $M.setTasksList(tasks); |
There was a problem hiding this comment.
business logic about how exactly we delete the task is better placed in the model
| if ($V.input.value.length > 0) { | ||
| if ($C.tasksListIsHidden) { | ||
| $C.tasksListIsHidden = false; | ||
| $V.toggleShowButton(); | ||
| } |
There was a problem hiding this comment.
better to avoid nested ifs
HW8(MVC)/scripts/Controller.js
Outdated
| clearTimeout(this.invalidInputTimer); | ||
|
|
||
| this.invalidInputTimer = setTimeout(() => { | ||
| $V.input.style.border = '1px solid #000'; |
HW8(MVC)/scripts/Controller.js
Outdated
| if (value.length > 0) { | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
just as a suggestion
return value.length > 0;
HW8(MVC)/scripts/Controller.js
Outdated
| } | ||
| } | ||
|
|
||
| anyTasksAlreadySaved() { |
There was a problem hiding this comment.
pls, use verbs for the name of the methods
| } | ||
|
|
||
| isFiltered() { | ||
| return localStorage.getItem('isFiltered') ?? 'false'; |
2 participants
HW was rewritten. Now it is written with classes.
TASK: Write a basic TODO list using the MVC pattern.
Requirements
✅ Base UI (you can use a bootstrap.css)
TODO units can be
✅ add
✅ mark as done
✅ unmark as done
✅ delete
Store data in RAM
✅ optional improvement - save in local storage
Optional improvements:
❗️ cover the code with unit tests
✅ add filter by completed and no