Homework#5 [Done] #3
Conversation
TigranKirakosov/index.js
Outdated
| }; | ||
| var ctrlToggleTask = function(event) { | ||
| var itemID, splitID, ID, icon; | ||
| if (event.target.className === DOM.uncheckedBtn || DOM.checkedBtn) { |
There was a problem hiding this comment.
if you wanna check that className of target equals to DOM.uncheckedBtn or DOM.checkedBtn you should write another condition, because current condition checks if className equals to DOM.uncheckedBtn or DOM.checkedBtn isn't equal to boolean false ( isn't empty string, undefined, 0 and so on)
There was a problem hiding this comment.
In current case, all I want to do is to check if event.target.className matches any of these certain classNames. Isn't that correct if there's no match the boolean result is false?
There was a problem hiding this comment.
yeah, it's correct, but I meant that your condition isn't correct - try in console if (1 === 2 || 3) console.log('oh!')
There was a problem hiding this comment.
Now I see. To be correct, I should've done code like this, I suppose: if (1 === 2 || 1 === 3) console.log('oh')
TigranKirakosov/index.js
Outdated
| icon = event.target; | ||
| } | ||
| if (itemID) { | ||
| splitID = itemID.split('-'); |
There was a problem hiding this comment.
move this logic to separated function getItemInfo that returns object with 2 fields - type and id
TigranKirakosov/index.js
Outdated
| DOM = UICtrl.getDOMStrings(); | ||
| tasks = storageCtrl.getFromLS(); | ||
| filterValue = document.querySelector(DOM.filterInput).value; | ||
| if (filterValue === 'no filter') { |
There was a problem hiding this comment.
move no filter value to some object with constants
| newTask = new Task(ID, text, deadline); | ||
| return newTask; | ||
| }, | ||
| addToLS: function(task) { |
There was a problem hiding this comment.
why both functions - addTask and addToLS are public api of storageController? - I think you need to extract all logic that is responsible for interactions with localStorage to separated controller/service and use it inside your storageController
There was a problem hiding this comment.
Will do that, valuable suggestion, should've separated the logic
| return newTask; | ||
| }, | ||
| addToLS: function(task) { | ||
| tasks.push(task); |
There was a problem hiding this comment.
why addToLS function makes more than just add task to LS? - in fact, you sync your internal state firstly and then add new state to LS. I think you need to move this logic to addTask function. current implementation of addTask function better to rename to smth like createTask
TigranKirakosov/index.js
Outdated
| this.iconClass = DOM.uncheckedBtn; | ||
| }; | ||
| Task.prototype.toggleState = function() { | ||
| if (this.isDone === 'undone') { |
There was a problem hiding this comment.
move strings done and undone to some object with constants
TigranKirakosov/index.js
Outdated
| filteredTasks = tasks.filter(function(task) { | ||
| return task.deadline == filterValue || task.isDone == filterValue; | ||
| }); | ||
| filteredTasks.forEach(function(task) { |
There was a problem hiding this comment.
move this logic to functions renderTasks and renderTask
TigranKirakosov/index.js
Outdated
| var html = ''; | ||
| tasks = storageCtrl.getFromLS(); | ||
| taskList = document.querySelector(DOM.taskList); | ||
| tasks.forEach(function(task) { |
There was a problem hiding this comment.
use just renderTasks function that I mentioned in comment above here instead of code duplication
TigranKirakosov/index.js
Outdated
| icon.className = DOMStrings.checkedBtn; | ||
| } | ||
| }, | ||
| filterTasks: function(tasks) { |
There was a problem hiding this comment.
why UI controller should know how to filter data? As I can see in your to do list UIController - it's classic view component. All what should do this controller - correctly display data to user. Filtering of the data should do controller (line 145) component and just put array of filtered tasks to UI controller
TigranKirakosov/index.js
Outdated
| .querySelector(DOM.submitBtn) | ||
| .addEventListener('click', ctrlAddTask); | ||
| document.addEventListener('keypress', function(event) { | ||
| if (event.keyCode === 13 || event.which === 13) { |
No description provided.