Skip to content

feat update: work began #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.

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 6 commits into
base: main
Choose a base branch
from
Open

feat update: work began #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3fd4239
feat update: work began
Ouriel91 May 15, 2022
4a9cb95
feat update: adds application main style
Ouriel91 May 15, 2022
fde52a4
feat update adds the main logic of todolist
Ouriel91 May 15, 2022
eee5951
feat update: added a-z and z-a filter
Ouriel91 May 17, 2022
6ea658f
feat update: adds edit option
Ouriel91 May 17, 2022
fae702d
feat update: addsempty state
Ouriel91 May 17, 2022
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
26 changes: 21 additions & 5 deletions src/ex1/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,30 @@
<!DOCTYPE html>
<html lang="en">
<head>
<title>Exercise 1</title>
<title>2DoList</title>
<link rel="stylesheet" href="style.css" />
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/5.15.3/css/all.min.css"/>
</head>
<body>

<h1>Todo list</h1>


<div class="container">
<div class="top">
<h1 class="title">2DoList</h1>
<select id="order-select" class="filter-select">
<option value="A-Z">A-Z</option>
<option value="Z-A">Z-A</option>
</select>
</div>
<div class="input-container">
Copy link

Choose a reason for hiding this comment

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

Great layout modeling! well done!

<input class="todo-input" id="todo-input" type="text" placeholder="Add your new todo">
<button class="add-button" id="add-todo-button"><i class="fas fa-plus"></i></button>
</div>
<div class="empty-todos-message" id="enter-todos"><h1>Enter Todos</h1></div>
<ul class="todo-list-container" id="todo-list"></ul>
<div class="clear-and-pending-container">
<span>You have <span id="sum-todos"></span> pending tasks</span>
<button class="clear-all-button" id="clear-all-todos-button">Clear All</button>
</div>
</div>
</body>
<script src="script.js"></script>
</html>
158 changes: 158 additions & 0 deletions src/ex1/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
const todoListElement = document.getElementById("todo-list")
const addTodoButton = document.getElementById("add-todo-button")
const todoInput = document.getElementById("todo-input")
const clearAllTodosButton = document.getElementById("clear-all-todos-button")
const sumTodos = document.getElementById("sum-todos")
const orderSelect = document.getElementById("order-select")
const enterTodos = document.getElementById("enter-todos")
Copy link

Choose a reason for hiding this comment

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

having all the elements outside here requires all the dom elements exist when this file is loaded, which might not be the case in complex web apps that dynamically change the dom depending on the view


let todoList = [] //array for storing data

showTodos()

todoInput.onkeyup = (e) => {
Copy link

Choose a reason for hiding this comment

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

it's a good practice to use the addEventListener method to add dom events

let enterValue = todoInput.value
if (enterValue.trim() !== ""){
Copy link

@SergeiSafrigin SergeiSafrigin May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

well done with this extra validation!

addTodoButton.classList.add("active")
addTodoButton.style.cursor = "pointer"
addTodoButton.style.opacity = 1

if(e.keyCode === 13){
Copy link

Choose a reason for hiding this comment

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

try to avoid "magic numbers", creating a const for this value will improve readability, for example:

const ENTER_KEY = 13; // at the top of the file

if (e.keyCode === ENTER_KEY) {...}

addTodo()
}
}
else{
addTodoButton.classList.remove("active")
Copy link

@SergeiSafrigin SergeiSafrigin May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

it's considered a good practice to hide the implementation of different actions to improve code readability, maintability & code-reuse, for example:

const isEmptyTodoValue = e.keyCode === ENTER_KEY;
if (isEmptyTodoValue) {
deactivateAddTaskBtn();
} else {
activateAddTaskBtn();
if (e.keyCode === ENTER_KEY) {
addTodo();
}
}

addTodoButton.style.opacity = 0.2
addTodoButton.style.cursor = "not-allowed"
}
}

addTodoButton.addEventListener("click", () => {
addTodo()
})

function addTodo(){
let enterValue = todoInput.value

if(enterValue.trim() === ""){
Copy link

Choose a reason for hiding this comment

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

the second time you check this -> could be moved to a function

alert("todo cannot be empty")
return
}

checkIfExistDataFromLS()
Copy link

Choose a reason for hiding this comment

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

I wonder why we have this check here as well

pushEnteredDataToLS(enterValue)
Copy link

Choose a reason for hiding this comment

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

Awesome! love it!!!

Copy link

Choose a reason for hiding this comment

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

Small bug: the enterValue is not trimmed before it's added

showTodos()

addTodoButton.classList.remove("active")
}

function checkIfExistDataFromLS(){
let dataFromLS = localStorage.getItem("new-todo")

if(dataFromLS === null){
todoList = []
}
else{
todoList = JSON.parse(dataFromLS)
Copy link

Choose a reason for hiding this comment

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

it's a bad practice to update some global variable from within a function, better use return instead :)

}
}

function pushEnteredDataToLS(enterValue){
todoList.push(enterValue)
localStorage.setItem("new-todo", JSON.stringify(todoList))
alert(`added new todo ${enterValue}`)
}

function showTodos() {
checkIfExistDataFromLS()
Copy link

Choose a reason for hiding this comment

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

why don't we do this only once when the app loads?

showMatchUiByTodosNumber()
createTodoListItems()
}

function showMatchUiByTodosNumber() {
sumTodos.textContent = todoList.length

if(todoList.length > 0){
clearAllTodosButton.classList.add("active")
clearAllTodosButton.style.cursor = "pointer"
enterTodos.style.display = "none"
}
else{
clearAllTodosButton.classList.remove("active")
clearAllTodosButton.style.cursor = "not-allowed"
enterTodos.style.display = "block"
}
}

function createTodoListItems() {
let listItems = ""
todoList.forEach((todo, index) => {
listItems += `<li>${todo}
<span class="delete" onclick="deleteTodo(${index})";>
<i class="fas fa-trash"></i>
</span>
<span class="edit" onclick="editTodo(${index})";>
Copy link

Choose a reason for hiding this comment

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

functions inside the html are considered a bad practice and can lead to many bugs, prefer using addEventListener

<i class="fas fa-pen"></i>
</span>
</li>
`
})

todoListElement.innerHTML = listItems
Copy link

Choose a reason for hiding this comment

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

cool idea!!! but has some downsides as you can't access the elements while creating them before they enter the dom


todoInput.value = ""
}

function deleteTodo(index) {

let dataFromLS = localStorage.getItem("new-todo")
todoList = JSON.parse(dataFromLS)
const removedTodo = todoList[index]
todoList.splice(index, 1) //remove one todo
alert(`removed new todo ${removedTodo}`)
localStorage.setItem("new-todo", JSON.stringify(todoList))
showTodos()
}

function editTodo(index) {
let editedValue = prompt('Edit todo:', todoList[index])
Copy link

@SergeiSafrigin SergeiSafrigin May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

BUG: if you cancel the edit, it will rename it to null

let dataFromLS = localStorage.getItem("new-todo")
todoList = JSON.parse(dataFromLS)
todoList[index] = editedValue
alert(`edited todo ${editedValue}`)
localStorage.setItem("new-todo", JSON.stringify(todoList))
showTodos()
}

clearAllTodosButton.addEventListener("click", () => {
let dataFromLS = localStorage.getItem("new-todo")

if(dataFromLS === null){
todoList = []
}
else{
todoList = JSON.parse(dataFromLS)
todoList = [] //initialize array again
}

alert('removed all todos')
localStorage.setItem("new-todo", JSON.stringify(todoList))
showTodos()
})

orderSelect.addEventListener('change', (e) => {
let dataFromLS = localStorage.getItem("new-todo")
todoList = JSON.parse(dataFromLS)
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 encapsulate all the local storage operations behind functions to avoid accessing it directly. Then you could also build wrappers that both fetch and save the todos, and handle that calls to the local storage functions when needed


if(e.target.value === "A-Z") {
todoList = todoList.sort()
Copy link

Choose a reason for hiding this comment

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

the sort method mutates the original array, therefore it's better to copy it beforehand ex: [...todoList.sort()];

As mutating the state directly (and complex data structures in general) is considered a bad practice as other parts of the code might be using it, not expecting it to suddenly change.
Read more on pure functions and the immutability approach in coding https://css-tricks.com/understanding-immutability-in-javascript/

Copy link
Author

Choose a reason for hiding this comment

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

Hi @SergeiSafrigin i fixed all the comments that you gave to me it was very helpfull

}
else{
todoList = todoList.sort().reverse()
}

localStorage.setItem("new-todo", JSON.stringify(todoList))
showTodos()
})

192 changes: 192 additions & 0 deletions src/ex1/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
@import url('https://fonts.googleapis.com/css2?family=Poppins:wght@200;300;400;500;600;700&display=swap');

* {
box-sizing: border-box;
margin: 0;
Copy link

@SergeiSafrigin SergeiSafrigin May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

in general, it's usually a bad practice to use the wildcard selector *, as it applies to all elements, rather than inheriting the style from a parent element (it still could be html or body) which are much easier to override when needed.
You can read more on CSS precedence here - https://jenkov.com/tutorials/css/precedence.html

You also have this cool library that normalizes the base/default styles across different browsers - https://necolas.github.io/normalize.css/

padding: 0;
font-family: 'Poppins', sans-serif;
}

body {
width: 100%;
height: 100vh;
Copy link

Choose a reason for hiding this comment

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

in order for the viewport styles (vh/vw) to apply correctly you need to add this meta tag to the HTML header

You can read more on css viewport here https://www.educba.com/css-viewport/

padding: 10px;
background: linear-gradient(to bottom, #68EACC 0%, #497BE8 100%);
}

.container {
background: #fff;
width: 60%;
max-height: 80%;
margin: 50px auto;
padding: 20px;
border-radius: 5px;
box-shadow: 0px 10px 15px rgba(0,0,0,0.1);
}

.top {
display: flex;
align-items: center;
justify-content: space-between;
}

.filter-select {
width: 80px;
padding: 10px;
border-radius: 5px;
border: none;
font-size: 16px;
font-weight: 500;
background-color: rgba(182, 128, 191, 0.3);
text-align: center;
}

.filter-select:focus{
outline: none;
}

.title {
font-size: 30px;
font-weight: bold;
}

.input-container {
display: flex;
margin: 20px 0;
height: 45px;
width: 100%;
justify-content: space-between;
}

.todo-input {
width: 85%;
height: 100%;
outline: none;
border-radius: 5px;
border: 1px solid #ccc;
padding-left: 15px;
font-size: 16px;
transition: all 0.3s ease;
}

.todo-input:focus {
border: 2px solid #5f56c4;
}

.add-button {
width: 10%;
min-width: 35px;
height: 100%;
Copy link

Choose a reason for hiding this comment

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

it's a good practice to align the base style of different components such as "button", to avoid code duplication, improve maintainability & have a consistent UI across the app.
You can add a button class with that base style and apply it to all the buttons.

(same goes for input and other components)

border: none;
color: #fff;
cursor: not-allowed;
font-size: 20px;
outline: none;
background: #8E49E8;
border-radius: 5px;
opacity: 0.6;
transition: all 0.3s ease;
}

.add-button.active, .clear-all-button.active {
Copy link

Choose a reason for hiding this comment

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

nice re-use!

opacity: 1;
transform: scale(0.98);
cursor: pointer;
}

.empty-todos-message {
position: relative;
background-image: url("https://www.itgovernance.eu/blog/de/wp-content/uploads/2017/04/list.jpg");
background-size: cover;
height: 40vh;
Copy link

Choose a reason for hiding this comment

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

viewport styles are considered a bad practice when used on inner components as it ignores the 'box' it's inside of, and uses the whole viewport as a reference, making it hard to maintain and forces you to change its size anytime the parent UI changes.

}

.empty-todos-message > h1 {
position: absolute;
top: 50%;
left: 50%;
transform: translateX(-50%) translateY(-50%);
z-index: 100;
Copy link

Choose a reason for hiding this comment

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

Much easier to achieve with flex-box and is considered a better practice in general - https://css-tricks.com/snippets/css/a-guide-to-flexbox/

}

.todo-list-container {
max-height: 250px;
overflow-y: auto;
Copy link

Choose a reason for hiding this comment

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

like!

}

.todo-list-container li {
Copy link

Choose a reason for hiding this comment

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

Using "li" here instead of a class makes it coupled to this specific solution. If you'll need to change li to a different element (ex: div) you'll have to make many changes in the code

position: relative;
list-style: none;
margin-bottom: 8px;
background: #f2f2f2;
border-radius: 5px;
padding: 10px;
overflow: hidden;
word-wrap: break-word;
cursor: default;
}

.todo-list-container li .delete{
position: absolute;
right: -40px;
top: 50%;
transform: translateY(-50%);
Copy link

@SergeiSafrigin SergeiSafrigin May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

the positioning here is very hard to maintain, as every change of the width will require adjusting the position of all the elements, flex-box will help you here as well

background: #e74c3c;
width: 40px;
text-align: center;
color: #fff;
padding: 10px;
border-radius: 0 5px 5px 0;
cursor: pointer;
transition: all 0.2s ease;
}

.todo-list-container li .edit{
position: absolute;
right: -80px;
top: 50%;
transform: translateY(-50%);
background: #563ce7;
width: 40px;
text-align: center;
color: #fff;
padding: 10px;
cursor: pointer;
transition: all 0.2s ease;
}

.todo-list-container li:hover .delete{
right: 0px;
}

.todo-list-container li:hover .edit{
right: 40px;
}

.todo-list-container li:hover {
background: rgba(139, 90, 139, 0.3);
}

.clear-and-pending-container {
display: flex;
align-items: center;
justify-content: space-between;
width: 100%;
margin-top: 20px;
}

.clear-all-button {
padding: 10px;
border-radius: 5px;
max-width: 90px;
border: none;
outline: none;
color: #fff;
font-weight: bold;
font-size: 16px;
background: #8E49E8;
cursor: not-allowed;
opacity: 0.6;
transition: all 0.3s ease;
word-wrap: break-word;
}