-
Notifications
You must be signed in to change notification settings - Fork 40
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
Exercises-1 pull req Dan Menahem #6
base: main
Are you sure you want to change the base?
Changes from all commits
730004d
c9cc47b
dae8f58
b3aef8d
49f0f70
c291d5b
d21409a
f0f34a1
5569207
de78f71
931ef53
9d3d245
ddb4482
c230c99
77ba7ef
acf582e
ffa366f
b0060ed
4435ae9
e0e08c3
aebf3e4
568035c
dec4ec5
415ce0a
9b06aca
91c5900
bc01042
84971ca
1cc8c5d
94202eb
2649797
cfe68e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,37 @@ | ||
<!DOCTYPE html> | ||
<html lang="en"> | ||
|
||
<head> | ||
<title>Exercise 1</title> | ||
<meta charset="UTF-8"> | ||
<meta http-equiv="X-UA-Compatible" content="IE=edge"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
<title>My Todo List</title> | ||
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css"> | ||
<link rel="icon" href="../images/todo-icon.png"> | ||
<link rel="stylesheet" href="style.css" /> | ||
</head> | ||
<body> | ||
|
||
<h1>Todo list</h1> | ||
|
||
<body onload="init()"> | ||
<div class="container"> | ||
<h1>My Todo list</h1> | ||
<div id="add-task"> | ||
<input type="text" id="taskInput" placeholder="Add your new todo task"> | ||
<button class="btn" id="addBtn" disabled>+</button> | ||
</div> | ||
<ul id="taskList"> | ||
</ul> | ||
<div class="placeHolder hidden"> | ||
<i class="fa fa-plus"></i> | ||
<p>Time to add your first task</p> | ||
</div> | ||
<div id="info"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good dom modeling, gj! |
||
<span id="peding-task-text">You have 0 pending tasks</span> | ||
<button class="btn" id="clearAllBtn" disabled>Clear All</button> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good job on reusing the btn class! |
||
<button class="btn" id="sortAllBtn" disabled>Sort All</button> | ||
</div> | ||
</div> | ||
|
||
</body> | ||
<script src="script.js"></script> | ||
</html> | ||
|
||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
const taskInput = document.querySelector(" #taskInput "); | ||
const addBtn = document.querySelector(" #addBtn "); | ||
const clearAllBtn = document.querySelector(" #clearAllBtn "); | ||
const sortAllBtn = document.querySelector(" #sortAllBtn "); | ||
const taskList = document.querySelector(" #taskList "); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
const placeHolder = document.querySelector(" .placeHolder ") | ||
const pedingTaskText = document.querySelector(" #peding-task-text ") | ||
|
||
const taskArr = JSON.parse(localStorage.getItem("tasks")) || []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice! |
||
|
||
|
||
//waiting for user input before enable the add button | ||
taskInput.addEventListener('keyup', () => addBtn.disabled = !taskInput.value) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extract the validation into a function as multiple sources need to use it :) |
||
|
||
// add a task when user press Enter key | ||
taskInput.addEventListener("keypress", (e) => { if (e.key === 'Enter') addNewTask(taskInput.value) }); | ||
|
||
//add click event listener to add button | ||
addBtn.addEventListener("click", () => addNewTask(taskInput.value)); | ||
|
||
//sort the tasks list add them to html list element and save them to the LocalStorage | ||
sortAllBtn.addEventListener("click", () => { | ||
taskArr.sort(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: taskArr = [...taskArr.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. |
||
taskList.innerHTML = ''; | ||
insertAllTasks(); | ||
localStorage.setItem("tasks", JSON.stringify(taskArr)); //add the updated task array to the localStorage | ||
}) | ||
|
||
//Clear the tasks list | ||
clearAllBtn.addEventListener("click", () => { | ||
taskArr.length = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a common usage, I would change it to taskArr = []; |
||
taskList.innerHTML = ''; //Clear the list of tasks element | ||
localStorage.setItem("tasks", JSON.stringify(taskArr)); //add the updated task array to the localStorage | ||
updateForm(); | ||
}) | ||
|
||
//function to add task and update the list and the localStorage | ||
function addNewTask(taskValue) { | ||
taskArr.push(taskValue); //add the user value to the task array | ||
taskList.appendChild(createLi(taskValue)); | ||
taskInput.value = ''; //clear the input after added | ||
addBtn.disabled = true; | ||
localStorage.setItem("tasks", JSON.stringify(taskArr)); //add the updated task array to the localStorage | ||
updateForm(); | ||
} | ||
|
||
// function to remove task that have been clicked | ||
function removeTask(li, taskValue) { | ||
taskArr.splice(taskArr.indexOf(taskValue), 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As task holds only the text value it is not unique, therefore you might be removing the wrong task from the list. Consider an alternative approach. hint: find a way to uniquely identify tasks |
||
taskList.removeChild(li); | ||
localStorage.setItem("tasks", JSON.stringify(taskArr)); //add the updated task array to the localStorage | ||
updateForm(); | ||
} | ||
|
||
//update the button and the placeHolder if necessary | ||
function updateForm() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider renaming the function to make it more descriptive or even separate into 2 different functions as you always know what you actually need to do beforehand. 1 for hiding the empty state and other for showing it |
||
pedingTaskText.innerHTML = "You have " + taskArr.length + " pending tasks" | ||
if (taskArr.length === 0) { //if there are no task any more disable the clear and sort button | ||
clearAllBtn.disabled = true; | ||
sortAllBtn.disabled = true; | ||
placeHolder.classList.remove("hidden"); | ||
} | ||
else { | ||
clearAllBtn.disabled = false; | ||
sortAllBtn.disabled = false; | ||
placeHolder.classList.add("hidden"); | ||
} | ||
} | ||
|
||
//function to insert the all tasks from the localStorage to the html list element | ||
function insertAllTasks() { | ||
taskArr.forEach((taskValue) => { | ||
taskList.appendChild(createLi(taskValue)); | ||
}); | ||
updateForm(); | ||
} | ||
|
||
//Function to create the li element and its childs and assign a click event to them | ||
function createLi(taskValue) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as the css point, let's decouple this by changing to something more general like createTaskElement/createTaskNode |
||
const li = document.createElement("li"); | ||
const span = document.createElement("span"); | ||
const i = document.createElement("i"); | ||
li.innerHTML = taskValue; | ||
i.classList.add("fa", "fa-trash-o"); | ||
span.appendChild(i); | ||
li.appendChild(span); | ||
span.onclick = (e) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prefer using the addEventListener instead of onclick directly |
||
e.stopPropagation(); //stop the parent elemnt to listen to click event | ||
removeTask(li, taskValue); | ||
}; | ||
li.onclick = () => alert(taskValue); | ||
li.classList.add('new-li'); | ||
return li; | ||
} | ||
|
||
//if there are some task in the localStorage insert them | ||
function init() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice use of init, I would add the loading of the localStorage and initializing the tasksList here as well |
||
if (taskArr.length != 0) insertAllTasks(); | ||
else placeHolder.classList.remove("hidden"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. code duplication, this could have been instead a call to showEmptyState()/ showEmptyStateIfNeeded()/toggleEmptyState() or something similar |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,163 @@ | ||
* { | ||
font-family: Alef, Arial, Helvetica, sans-serif; | ||
font-size: 20px; | ||
margin: 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 also have this cool library that normalizes the base/default styles across different browsers - https://necolas.github.io/normalize.css/ |
||
padding: 0; | ||
} | ||
|
||
body { | ||
display: flex; | ||
justify-content: center; | ||
align-items: center; | ||
height: 100vh; | ||
width: 100%; | ||
background: linear-gradient(to top, rgb(47, 255, 102) 0%, rgb(0, 200, 255) 100%); | ||
} | ||
|
||
h1 { | ||
font-size: 35px; | ||
} | ||
|
||
|
||
.container { | ||
background-color: white; | ||
border-radius: 10px; | ||
width: 100%; | ||
max-width: 500px; | ||
padding: 25px; | ||
} | ||
|
||
.btn[disabled] { | ||
opacity: 0.3; | ||
box-shadow: none; | ||
border: none; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. duplicate? isn't it already in .btn? |
||
} | ||
|
||
.btn[disabled]:hover { | ||
cursor: not-allowed; | ||
box-shadow: none; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. duplicate? isn't it already in .btn[disabled]? (move below .btn:hover) |
||
} | ||
|
||
.btn { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's a good practice to put all the different variants of a style after the base is defined so the variants will get a priority and could override the base class with ease and without the need for !important |
||
height: 100%; | ||
text-align: center; | ||
color: white; | ||
border: none; | ||
margin-left: 10px; | ||
border-radius: 8px; | ||
cursor: pointer; | ||
background-color: rgb(30, 75, 254); | ||
} | ||
|
||
.btn:hover { | ||
background-color: rgb(0, 51, 255); | ||
box-shadow: 0.5px 0.5px 10px 0.5px grey; | ||
} | ||
|
||
|
||
.placeHolder { | ||
display: flex; | ||
justify-content: center; | ||
align-items: center; | ||
flex-direction: column; | ||
color: greenyellow; | ||
font-size: 100px; | ||
opacity: 0.3; | ||
padding: 60px; | ||
border: 1px solid rgb(99, 99, 99); | ||
border-radius: 8px; | ||
} | ||
|
||
.hidden { | ||
display: none; | ||
} | ||
|
||
.placeHolder p { | ||
margin-top: 5px; | ||
color: black; | ||
} | ||
|
||
#addBtn { | ||
width: 60px; | ||
font-size: 30px; | ||
} | ||
|
||
#add-task { | ||
display: flex; | ||
width: 100%; | ||
height: 50px; | ||
margin: 30px 0; | ||
} | ||
|
||
#add-task input { | ||
width: 90%; | ||
height: 100%; | ||
border: 1px solid #ccc; | ||
border-radius: 8px; | ||
padding-left: 15px; | ||
outline: none; | ||
} | ||
|
||
|
||
#taskList { | ||
max-height: 260px; | ||
overflow-y: auto; | ||
} | ||
|
||
|
||
#taskList li { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
list-style: none; | ||
line-height: 45px; | ||
margin-bottom: 8px; | ||
background-color: rgb(237, 237, 237); | ||
border-radius: 8px; | ||
padding-left: 15px; | ||
position: relative; | ||
overflow: hidden; | ||
} | ||
|
||
#taskList li span { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same goes for the span here better to use a class name. Also for readability, a descriptive classname will tell me on which element this style is targeting |
||
position: absolute; | ||
right: -45px; | ||
background-color: red; | ||
color: white; | ||
width: 40px; | ||
text-align: center; | ||
border-radius: 0 8px 8px 0; | ||
cursor: pointer; | ||
transition: all 0.3s ease; | ||
} | ||
|
||
#taskList li:hover span { | ||
right: 0; | ||
} | ||
|
||
#info { | ||
display: flex; | ||
justify-content: space-around; | ||
width: 100%; | ||
margin-top: 20px; | ||
align-items: center; | ||
} | ||
|
||
#clearAllBtn, | ||
#sortAllBtn { | ||
padding: 10px; | ||
} | ||
|
||
@keyframes append-animate { | ||
from { | ||
transform: translateX(-100%); | ||
opacity: 1; | ||
} | ||
|
||
to { | ||
transform: translateX(0%); | ||
opacity: 1; | ||
} | ||
} | ||
|
||
/* animate new box */ | ||
.new-li { | ||
animation: append-animate .4s linear; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
class ItemManager { | ||
constructor() { | ||
this.taskArr = JSON.parse(localStorage.getItem("tasks")) || []; | ||
} | ||
|
||
addNewTask(taskValue) { | ||
this.taskArr.push(taskValue); //add the user value to the task array | ||
this.updateLocalStorage(this.taskArr); | ||
} | ||
|
||
removeTask(taskValue) { | ||
this.taskArr = this.taskArr.filter(task => task !== taskValue); | ||
this.updateLocalStorage(this.taskArr); | ||
} | ||
|
||
sortArr() { | ||
this.taskArr.sort(); | ||
this.updateLocalStorage(this.taskArr); | ||
} | ||
|
||
clearArr() { | ||
this.taskArr.length = 0; | ||
this.updateLocalStorage(this.taskArr); | ||
} | ||
|
||
updateLocalStorage(valueToUpdate) { | ||
localStorage.setItem("tasks", JSON.stringify(valueToUpdate)); //add the updated task array to the localStorage | ||
} | ||
} |
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.
nice