Pr exc1 #26
Pr exc1 #26
Conversation
| /> | ||
| <div class="style"> | ||
| <head> | ||
| <title>Exercise 1</title> |
There was a problem hiding this comment.
you should choose your own title so the tab name won't be just exercise 1
| <div id="listToDo" class="listToDo"></div> | ||
| <div id="footer" class="footer"> |
There was a problem hiding this comment.
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
| @@ -0,0 +1,64 @@ | |||
| const listToDo = document.querySelector(".listToDo"); | |||
There was a problem hiding this comment.
you didn't implement the alert when clicking on a task
| const listToDo = document.querySelector(".listToDo"); | ||
| const count = document.querySelector(".count"); | ||
| const buttonClearAll = document.querySelector(".buttonClearAll"); | ||
| const newToInput = document.querySelector(".newToInput"); | ||
| const buttonAdd = document.querySelector(".buttonAdd"); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
why do you need to call it on initialization?
| } | ||
| } | ||
|
|
||
| function add() { |
There was a problem hiding this comment.
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
| } | ||
|
|
||
| function add() { | ||
| if (document.querySelector("#newToDo input").value.length == 0) { |
There was a problem hiding this comment.
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
| this.parentNode.style.opacity = "0"; | ||
| count.textContent--; | ||
| clickButtonClearAll(); | ||
| setTimeout(() => this.parentNode.remove()(), 200); |
There was a problem hiding this comment.
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
| } | ||
| } | ||
|
|
||
| function clickButtonClearAll() { |
There was a problem hiding this comment.
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
| @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)); | ||
| } | ||
| } |
There was a problem hiding this comment.
do you know what is it? do you need it?
There was a problem hiding this comment.
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
No description provided.