-
Notifications
You must be signed in to change notification settings - Fork 9
TaskList. Anastasiya Santalova #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
TaskList. Anastasiya Santalova #2
Conversation
…d saving tasks after reloading
…e functional and visual parts.
Anastasiya Santalova/app.js
Outdated
var taskList; | ||
var localTaskList = JSON.parse(localStorage.getItem('myArr')); | ||
|
||
if (localTaskList.length == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would fail while getting length
if I don't have such item in local storage.
Also, use triple equals operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
Anastasiya Santalova/app.js
Outdated
break; | ||
case 'today': | ||
taskList.forEach(function (curr) { | ||
console.log(compareDates(curr.deadline)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be removed I suppose
Anastasiya Santalova/app.js
Outdated
// * CHANGESTAT: change and save task status (completed / not completed) | ||
|
||
|
||
var list = (function (){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add whitespace before curly brace
In future I'd recommend to use linters e.g. ESLint
which have warning messages if you violate the styleguide and also some tools like Prettier
which could fix those problems
Anastasiya Santalova/app.js
Outdated
} | ||
|
||
return { | ||
create: function (text,deadline) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate function args with whitespace
Anastasiya Santalova/app.js
Outdated
var ind; | ||
|
||
taskList.forEach(function (curr, i) { | ||
if (curr.id == id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use triple equals operator
Anastasiya Santalova/SCSS/app.scss
Outdated
|
||
border-bottom: 1px dashed lightgray; | ||
|
||
//@include borderGray(dashed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented line please
Anastasiya Santalova/app.js
Outdated
delete: function (id) { | ||
var ind; | ||
|
||
taskList.forEach(function (curr, i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of forEach + splice
combination I'd use filter
Anastasiya Santalova/app.js
Outdated
div.appendChild(delSpan); | ||
delSpan.appendChild(delButton); | ||
|
||
delButtonOnclick(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use camelCase please: delButtonOnClick
. The same note for the function below
Anastasiya Santalova/app.js
Outdated
}, | ||
|
||
changestat: function(id) { | ||
taskList.forEach(function (curr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use find
since it will stop after finding needed element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know find can be used only in ES6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right, my bad :)
Anastasiya Santalova/app.js
Outdated
list.push(newTask); | ||
list.show(); | ||
} | ||
};; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excessive semicolon
No description provided.