CYF-ITP-South Africa | Serna Malala | Module-Data-Groups | Week 3 Todo List #287
Conversation
sernamalala
added
📅 Sprint 3
| listItem.style.background = "lightblue"; | ||
| listItem.style.padding = "1em"; | ||
| listItem.style.display = "inline-block"; | ||
| listItem.style.margin = "0.5em"; | ||
| listItem.textContent = todoItem.task; | ||
| list.style.margin = "0 auto"; | ||
| list.style.backgroundColor = "#FCC4AB"; | ||
| body.style.textAlign = "center" |
There was a problem hiding this comment.
Why not set CSS styles in styles.css? That way, whenever we need to change the view we don't have to change the code.
| list.style.backgroundColor = "#FCC4AB"; | ||
| body.style.textAlign = "center" | ||
|
|
||
| list.appendChild(breakLine); |
There was a problem hiding this comment.
Adding <br> to a list would result in creating something like
<ul>
<li>....</li>
<br>
<li>...</li>
<br>
</ul>
which is considered an error in HTML.
Can you think of a better way to create space between list items using only CSS?
|
|
||
| let todo = {}; | ||
|
|
||
| if (inputFieldValue.length > 0) { |
There was a problem hiding this comment.
Do you want to accept any input containing only space characters?
|
|
||
| if (todoID) { | ||
| todos.splice(parseInt(todoID), 1); | ||
| listItem.remove(); |
There was a problem hiding this comment.
Is line 76 necessary? The next statement, populateTodoList(), will update the whole UI by reconstructing all the elements that make up UI.
|
|
||
|
|
||
| deleteButton.addEventListener("click", function () { | ||
| const listItem = this.closest("li"); |
| if (todoID && todos[parseInt(todoID)].completed === false) { | ||
| todos[parseInt(todoID)].completed = true; | ||
| listItem.style.textDecoration = "line-through"; | ||
|
|
||
| } | ||
| else { | ||
| todos[parseInt(todoID)].completed = false; | ||
| listItem.style.textDecoration = "none"; | ||
| } |
There was a problem hiding this comment.
Why not just call populateTodoList() at line 59 and 64 to update the whole UI?
Any chance todoID is undefined or an empty string?
The code at lines 57-65 could possibly be reduced to 2-3 lines of code.
| checkMark.id = todoButtonsIndex; | ||
| deleteButton.id = todoButtonsIndex; | ||
| todoButtonsIndex++; |
There was a problem hiding this comment.
- These id values are not unique.
- Why do you want to assign id to these two buttons? Where are these id values being used?
- If you don't need these id values, you probably won't need
todoButtonsIndex.
|
|
||
| let todoButtonsIndex = 0; | ||
|
|
||
| let todoButtons = `<span class="badge bg-primary rounded-pill"> |
There was a problem hiding this comment.
If todoButtons does not need to be reassigned a different value later, it is better to declare it using const.
cjyuan
added
Reviewed
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
I created a todolist app that dynamically adds todo items to the screen using javascript. The check mark and delete buttons also work to strikethrough and delete the items.
Questions
Ask any questions you have for your reviewer.