Skip to content

Vladimir Kulikov #4

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Vladimir Kulikov #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4d13f1f
Vladimir Kulikov
kulikov98 Aug 5, 2019
82a23fc
code improvements
kulikov98 Aug 5, 2019
db74f43
some code improvements
kulikov98 Aug 7, 2019
cbfa25f
add DOMSelectors obj
kulikov98 Aug 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.DS_Store
20 changes: 20 additions & 0 deletions Vladimir Kulikov/custom.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
h1, h2 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

margin: 40px 0 20px 0;
}

.add-task-block {
background: #eee;
padding: 30px;
}

.dropdown {
margin-bottom: 10px;
}

.filters {
padding: 20px 0;
}

select {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with previous. Select should have class

margin-right: 20px;
}
78 changes: 78 additions & 0 deletions Vladimir Kulikov/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<!DOCTYPE html>
<html lang="en">

<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<meta http-equiv="X-UA-Compatible" content="ie=edge">
<title>TO-DO list</title>
<link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css"
integrity="sha384-ggOyR0iXCbMQv3Xipma34MD+dH/1fQ784/j6cY/iJTQUOhcWr7x9JvoRxT2MZw1T" crossorigin="anonymous">
<link rel="stylesheet" href="custom.css">
</head>

<body>
<div class="container">
<h1>TO-DO List</h1>
<div class="add-task-block">
<form>
<div class="form-row">
<div class="form-group col-md-6">
<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">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Field cannot be empty.
</div>
<small id="taskHelp" class="form-text text-muted">Briefly describe the task.</small>
</div>
<div class="form-group col-md-6">
<label for="deadline">Deadline</label>
<input type="date" class="add-task-deadline form-control" id="deadline"
aria-describedby="deadlineHelp">
<small id="deadlineHelp" class="form-text text-muted">Format: dd/mm/yyyy</small>
</div>
</div>
<button type="submit" class="add-task-btn btn btn-primary">Add task</button>
</form>
</div>
<h2>Tasks</h2>
<div class="filters">
<span>Sort by status: </span>
<select class="task-filter status-filter" name="status">
<option value="all">all</option>
<option value="completed">completed</option>
<option value="incomplete">incomplete</option>
</select>
<span>Sort by deadline: </span>
<select class="task-filter deadline-filter" name="deadline">
<option value="all">all</option>
<option value="until-tomorrow">until-tomorrow</option>
<option value="in-a-week">in a week</option>
</select>
</div>
<table class="table">
<thead>
<tr>
<th scope="col">Task</th>
<th scope="col">Deadline</th>
<th scope="col"></th>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strange that the column doesn't have a name. You can call it 'Actions' or something like this

</tr>
</thead>
<tbody class="task-list"></tbody>
</table>
</div>

<script src="script.js"></script>
<script src="https://code.jquery.com/jquery-3.3.1.slim.min.js"
integrity="sha384-q8i/X+965DzO0rT7abK41JStQIAqVgRVzpbzo5smXKp4YfRvH+8abtTE1Pi6jizo"
crossorigin="anonymous"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.14.7/umd/popper.min.js"
integrity="sha384-UO2eT0CpHqdSJQ6hJty5KVphtPhzWj9WO1clHTMGa3JDZwrnQq4sF86dIHNDz0W1"
crossorigin="anonymous"></script>
<script src="https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/js/bootstrap.min.js"
integrity="sha384-JjSmVgyd0p3pXB1rRibZUAYoIIy6OrQ6VrjIEaFf/nJGzIxFDsf4x0xIM+B07jRM"
crossorigin="anonymous"></script>
</body>

</html>
203 changes: 203 additions & 0 deletions Vladimir Kulikov/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
var todo = (function () {

var DOMSelectors = {
addTaskInput: '.add-task-text',
addTaskDeadlineInput: '.add-task-deadline',
statusFilterSelect: '.status-filter',
deadlineFilterSelect: '.deadline-filter',
tasksListTbody: '.task-list',
addTaskBtn: '.add-task-btn',
removeTaskBtns: '.remove-task-btn',
completeTaskBtns: '.complete-task-btn',
filters: '.task-filter'
};

var run = function () {
if (!localStorage.getItem('todoList')) localStorage.setItem('todoList', "[]");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to write like this (even when you have only one action):

if (...) {
    ...
}

https://stackoverflow.com/questions/4797286/are-braces-necessary-in-one-line-statements-in-javascript

if (!localStorage.getItem('todoListId')) localStorage.setItem('todoListId', 1);
render();
}

var addTask = function (e) {
e.preventDefault();
var task = document.querySelector(DOMSelectors.addTaskInput);
var deadline = document.querySelector(DOMSelectors.addTaskDeadlineInput);

if (validate(task, deadline)) {
var tasks = getTasks();
var id = genId();
tasks.push({ id: id, task: task.value, deadline: deadline.value, completed: false });
saveTasks(tasks);
task.value = "";
deadline.value = "";
render();
Copy link

@mariia-zu mariia-zu Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

var removeTask = function () {
var id = this.dataset.remove;
var tasks = getTasks();
var updatedTasks = tasks.filter(function (task) {
return task.id != id;
});
saveTasks(updatedTasks);
render();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a user removes a task, you should remove the listeners on click and remove for according buttons.
You have to remove only one node (not rerender everything)

}

var completeTask = function () {
var id = this.dataset.complete;
var tasks = getTasks();
var updatedTasks = tasks.map(function (task) {
if (task.id === id) task.completed = true;
return task;
});
saveTasks(updatedTasks);
render();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here also only one task has been changed

}

var getFilteredTasks = function () {
var tasks = getTasks();
var filters = [filterByStatus, filterByDeadline];

var filteredTasks = filters.reduce(function (acc, filter) {
acc = filter(acc);
return acc;
}, tasks);

return filteredTasks;
}

var filterByStatus = function (tasks) {
var filterParam = document.querySelector(DOMSelectors.statusFilterSelect).value;
switch (filterParam) {
case 'completed':
completedTasks = tasks.filter(function (task) { return task.completed == true });
return completedTasks;
case 'incomplete':
incompleteTasks = tasks.filter(function (task) { return task.completed == false });
return incompleteTasks;
default:
return tasks;
}
}

var filterByDeadline = function (tasks) {
var filterParam = document.querySelector(DOMSelectors.deadlineFilterSelect).value;

if (filterParam == 'until-tomorrow') {
var afterTomorrow = new Date(new Date().setDate(new Date().getDate() + 2));
var untilTomorrowTasks = tasks.filter(function (task) {
deadline = new Date(Date.parse(task.deadline));
if (afterTomorrow.getDate() > deadline.getDate() && deadline.getDate() >= new Date().getDate()) {
return task;
}
});
return untilTomorrowTasks;
}
if (filterParam == 'in-a-week') {
var nextWeek = new Date(new Date().setDate(new Date().getDate() + 8));
var weekTasks = tasks.filter(function (task) {
deadline = new Date(Date.parse(task.deadline));
if (nextWeek.getDate() > deadline.getDate() && deadline.getDate() >= new Date().getDate()) {
return task;
}
});
return weekTasks;
}
return tasks;
}

var validate = function () {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable should have a name like isValid.
https://www.samanthaming.com/tidbits/34-better-boolean-variable-names
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-type-members

return true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If in all cases a function returns true, this function can be removed, because this code is absolutely useless.
If you don't want to remove this function, you should at least check a length of a task name

}

var appendAll = function (parent, childs) {
childs.forEach(function (elem) {
parent.appendChild(elem);
});
return parent;
};

var createNode = function (task) {
var tr = document.createElement('tr');
tr.setAttribute('data-id', task.id);
var text = document.createElement('td');
text.textContent = task.task;
var deadline = document.createElement('td');
deadline.textContent = task.deadline || "-";
var controls = document.createElement('td');

var removeBtn = document.createElement('button');
removeBtn.classList.add('btn', 'btn-outline-danger', 'remove-task-btn');
removeBtn.setAttribute('type', 'button');
removeBtn.setAttribute('data-remove', task.id);
removeBtn.textContent = 'Remove';

if (task.completed) {
tr.classList.add('table-success');
var controls = appendAll(controls, [removeBtn]);
} else {
var completeBtn = document.createElement('button');
completeBtn.classList.add('btn', 'btn-outline-primary', 'complete-task-btn');
completeBtn.setAttribute('type', 'button');
completeBtn.setAttribute('data-complete', task.id);
completeBtn.textContent = 'Mark as complete';
var controls = appendAll(controls, [completeBtn, removeBtn]);
}

var node = appendAll(tr, [text, deadline, controls]);
return node;
}

var render = function (filteredTasks) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter filteredTasks should be removed, because it isn't used anywhere in the function

var parentNode = document.querySelector(DOMSelectors.tasksListTbody);
while (parentNode.firstChild) {
parentNode.removeChild(parentNode.firstChild);
}
var tasks = getFilteredTasks();

tasks.forEach(function (task) {
parentNode.appendChild(createNode(task));
});

addEventListeners();
}

var addEventListeners = function () {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should be invoked only one time or you should change it.
This function overrides previous event listeners each time when user create, update or delete a task, you don't need to do it, it just makes your app slower

var addTaskBtn = document.querySelector(DOMSelectors.addTaskBtn);
addTaskBtn.addEventListener('click', addTask);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it enough to create this listener one time?
Now you invoke addEventListener for any user action, it's a bad idea.
Nothing happens with the button Add task. You have to add listener when the page is loaded, that's all.


var removeTaskBtns = document.querySelectorAll(DOMSelectors.removeTaskBtns);
removeTaskBtns.forEach(function (btn) {
btn.addEventListener('click', removeTask);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with delete button

});

var filters = document.querySelectorAll(DOMSelectors.filters);
filters.forEach(function (filter) {
filter.addEventListener('change', render);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with the button Add task

});
}

var getTasks = function () {
return JSON.parse(localStorage.todoList);
}

var saveTasks = function (tasks) {
localStorage.todoList = JSON.stringify(tasks);
}

var genId = function () {
var id = localStorage.todoListId;
localStorage.todoListId = +localStorage.todoListId + 1;
return id;
}

return { run: run };
})();

todo.run();