Skip to content
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

Pr exc1 #26

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Pr exc1 #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c9abadb
feac: ex1
shirlyba94 May 19, 2022
1277f81
exc1-template
amirbardugo May 23, 2022
8db957e
exc1
amirbardugo May 23, 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
46 changes: 35 additions & 11 deletions src/ex1/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,38 @@
<!DOCTYPE html>
<html lang="en">
<head>
<title>Exercise 1</title>
<link rel="stylesheet" href="style.css" />
</head>
<body>

<h1>Todo list</h1>


</body>
<script src="script.js"></script>
<link
rel="stylesheet"
href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css"
/>
<div class="style">
<head>
<title>Exercise 1</title>
Copy link
Author

Choose a reason for hiding this comment

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

you should choose your own title so the tab name won't be just exercise 1

<link rel="stylesheet" href="style.css" />
</head>
<body>
<div class="card">
<h1>Todo list</h1>
<div id="newToDo">
<input
type="text"
name="newToInput"
id="newToInput"
value=""
placeholder="Add your new todo"
/>
<button type="button" id="buttonAdd" class="buttonAdd">+</button>
</div>
<div id="listToDo" class="listToDo"></div>
<div id="footer" class="footer">
Comment on lines +25 to +26
Copy link
Author

Choose a reason for hiding this comment

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

you don't have to give id and class to all of your elements, usually id makes more sense when you are sure you'll have only one of this element and you want to access it easily from js, in that case the class is quite redundant here

<h3 class="footer">
You have <span class="count"></span> pending tasks
</h3>
<button type="button" id="buttonClearAll" class="buttonClearAll">
Clear All
</button>
</div>
</div>
</body>
<script src="script.js"></script>
</div>
</html>
64 changes: 64 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,64 @@
const listToDo = document.querySelector(".listToDo");
Copy link
Author

Choose a reason for hiding this comment

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

you didn't implement the alert when clicking on a task

const count = document.querySelector(".count");
const buttonClearAll = document.querySelector(".buttonClearAll");
const newToInput = document.querySelector(".newToInput");
const buttonAdd = document.querySelector(".buttonAdd");
Comment on lines +1 to +5
Copy link
Author

Choose a reason for hiding this comment

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

if you have id's on some of them might be better to use the id as the selector - i.e "#listToDo"


var removeSVG = '<i class="fa fa-trash"></i>';
count.textContent = 0;

clickButtonClearAll();
Copy link
Author

Choose a reason for hiding this comment

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

why do you need to call it on initialization?


document.addEventListener("keydown", addEnter);
buttonAdd.addEventListener("click", add);
buttonClearAll.addEventListener("click", deleteListToDo);

function addEnter(event) {
if (
event.code === "Enter" &&
document.querySelector("#newToDo input").value.length != 0
Copy link
Author

Choose a reason for hiding this comment

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

you could also used more specific id like #newToInput, usually better to be as specific as possible when you'r trying to get some element

Copy link
Author

Choose a reason for hiding this comment

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

why do you have a validation here if you have another on on add function?

)
add();
}

function deleteListToDo() {
if (confirm("Are you sure??") == true) {
document.querySelector("#listToDo").innerHTML = "";
count.textContent = 0;
clickButtonClearAll();
}
}

function clickButtonClearAll() {
Copy link
Author

Choose a reason for hiding this comment

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

if i had to guess by the function name I would say that this is what happens when you click the clear all,
I would go with something like setIsActiveClearButton

if (count.textContent > 0) {
buttonClearAll.classList.add("active");
} else {
buttonClearAll.classList.remove("active");
}
}

function add() {
Copy link
Author

Choose a reason for hiding this comment

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

try to give some more indicative name to the functions, when things will start to get complicated it will help you (and whoever will read your code) a lot

if (document.querySelector("#newToDo input").value.length == 0) {
Copy link
Author

Choose a reason for hiding this comment

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

you are querying this element a lot, its better to do it once at the start and use some const like you did with other elements, this is better in terms of performance (the browser doesn't have to search for that element over and over again) and in terms of later changes and refactors, you'll have only one place the depends on the input id

alert("Please Enter new todo");
} else {
var div = document.createElement("div");
div.classList.add("todo");
var item = document.createElement("span");
item.innerText = document.querySelector("#newToDo input").value;
var remove = document.createElement("button");
remove.classList.add("delete");
remove.innerHTML = removeSVG;
remove.addEventListener("click", function () {
this.parentNode.style.opacity = "0";
count.textContent--;
clickButtonClearAll();
setTimeout(() => this.parentNode.remove()(), 200);
Comment on lines +52 to +55
Copy link
Author

Choose a reason for hiding this comment

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

is that for the animation? cool idea but it introduce a bug, if I double click remove button the counter is getting out of sync and you can get to this situation
image

});
div.appendChild(remove);
div.appendChild(item);
listToDo.insertBefore(div, listToDo.childNodes[0]);
document.getElementById("newToInput").value = "";
count.textContent++;
clickButtonClearAll();
}
}
266 changes: 266 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,266 @@
* {
box-sizing: border-box;
font-family: "Lucida Console", "Courier New", monospace;
font-family: "Franklin Gothic Medium";
}

.style {
padding: 45px;
width: 100%;
height: 100vh;
background-image: linear-gradient(120deg, lightblue, rgb(45, 111, 133));
}

.h2 {
font-size: 16px;
font-weight: bold;
}

.hr {
border: 1px solid #f1f1f1;
margin-bottom: 25px;
}
input[type="text"] {
padding-left: 15px;
width: 79%;
padding: 15px;
margin: 5px 0 22px 0;
border: none;
background: #f1f1f1;
}

input[type="text"]:focus {
background-color: #ddd;
outline: none;
}

.footer {
display: inline;
width: 80px;
height: 90px;
}
.footer .h3 {
font-size: 4px;
margin: 30px;
}
.footer .buttonClearAll {
background-color: #0095ff;
border: 1px solid transparent;
border-radius: 3px;
box-shadow: rgba(255, 255, 255, 0.4) 0 1px 0 0 inset;
box-sizing: border-box;
color: #fff;
cursor: pointer;
font-size: 10px;
padding: 6px 18px;
pointer-events: none;
opacity: 0;
display: inline-block;
}
.footer .buttonClearAll:hover,
.footer .buttonClearAll:focus {
background-color: #07c;
pointer-events: auto;
}

.footer .buttonClearAll:focus {
box-shadow: 0 0 0 4px rgba(0, 149, 255, 0.15);
pointer-events: auto;
}

.footer .buttonClearAll.active {
box-shadow: none;
opacity: 1;
pointer-events: auto;
align-items: left;
margin: 10px;
}
.buttonAdd {
background-color: #0095ff;
border: 1px solid transparent;
border-radius: 3px;
box-shadow: rgba(255, 255, 255, 0.4) 0 1px 0 0 inset;
box-sizing: border-box;
color: #fff;
cursor: pointer;
font-size: 19px;
font-weight: 400;
line-height: 1.15385;
margin: 0;
outline: none;
padding: 10px;
position: relative;
text-align: center;
text-decoration: none;
user-select: none;
-webkit-user-select: none;
touch-action: manipulation;
vertical-align: baseline;
white-space: nowrap;
}

.buttonAdd:hover,
.buttonAdd:focus {
background-color: #07c;
}

.buttonAdd:focus {
box-shadow: 0 0 0 4px rgba(0, 149, 255, 0.15);
}

.card {
box-shadow: 0 4px 8px 0 rgba(0, 0, 0, 0.2);
transition: 0.3s;
width: 85%;
margin: auto;
width: 30%;
border: 3px solid rgb(0, 62, 128);
padding: 10px;
background-color: rgb(255, 255, 255);
align-items: center;
overflow: clip;
}

.card:hover {
box-shadow: 0 8px 16px 0 rgba(0, 0, 0, 0.2);
}

.todo {
background-color: #c5e1e6;
height: 50px;
margin-bottom: 8px;
padding: 5px 10px;
border-radius: 5px;
align-items: center;
justify-content: space-between;
border: 1px solid #939697;
cursor: pointer;
width: 90%;
opacity: 1;
-webkit-transition: opacity 1000ms linear;
transition: opacity 1000ms linear;
word-break: break-all;
overflow: auto;
overflow: hidden;
animation: slideInRight;
animation-duration: 0.5s;
}
.todo span {
display: block;
max-width: 2200px;
-ms-word-break: break-all;
word-break: break-all;
word-break: break-word;
opacity: 1;
}

.todo:hover,
.todo:focus {
background-color: #7cafb8;
opacity: 1;
animation: slideInRight;
animation-duration: 0.5s;
}

.todo:focus {
box-shadow: 0 0 0 4px rgba(167, 207, 236, 0.15);
}

.todo:hover .delete {
opacity: 1;
animation: slideInRight;
animation-duration: 0.5s;
}

.todo .delete {
background-color: #0095ff;
border: 1px solid transparent;
border-radius: 3px;
box-shadow: rgba(255, 255, 255, 0.4) 0 1px 0 0 inset;
box-sizing: border-box;
color: #fff;
cursor: pointer;
font-size: 13px;
font-weight: 400;
line-height: 1.15385;
margin: 0;
outline: none;
padding: 10px;
position: relative;
text-align: center;
text-decoration: none;
user-select: none;
-webkit-user-select: none;
touch-action: manipulation;
vertical-align: baseline;
white-space: nowrap;
float: right;
opacity: 0;
}

.todo .delete:hover,
.delete:focus {
background-color: #07c;
opacity: 1;
}

.todo .delete:focus {
box-shadow: 0 0 0 4px rgba(0, 149, 255, 0.15);
opacity: 1;
}
.newToDo {
display: inline-block;
width: 10%;
text-align: center;
}
.button {
background-color: #0095ff;
border: 1px solid transparent;
border-radius: 3px;
box-shadow: rgba(255, 255, 255, 0.4) 0 1px 0 0 inset;
box-sizing: border-box;
color: #fff;
cursor: pointer;
display: inline-block;
font-family: "Times New Roman", Times, serif;
font-size: 13px;
font-weight: 400;
line-height: 1.15385;
margin: 0;
outline: none;
padding: 10px;
position: relative;
text-align: center;
text-decoration: none;
user-select: none;
-webkit-user-select: none;
touch-action: manipulation;
vertical-align: baseline;
white-space: nowrap;
}

.button:hover,
.button:focus {
background-color: #07c;
}

.button:focus {
box-shadow: 0 0 0 4px rgba(0, 149, 255, 0.15);
}

@media screen and (max-width: 100px) {
.col-10,
.col-90,
input[type="submit"] {
width: 30%;
margin-top: 0;
}
}
@media (min-width: 100px) {
.style {
margin-right: 100px;
width: 100%;
height: 100vh;
background-image: linear-gradient(120deg, lightblue, rgb(45, 111, 133));
}
}
Comment on lines +251 to +266
Copy link
Author

Choose a reason for hiding this comment

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

do you know what is it? do you need it?

Copy link
Author

Choose a reason for hiding this comment

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

overall it seems like a lot of the CSS here is not relevant and copied, its great to get inspired but we usually want to be in max control and understanding of each line in our code,
we don't want to have redundant code that is not in use or we even don't know its purpose