Ex1 #1
Ex1 #1
Conversation
There was a problem hiding this comment.
Overall:
- Very nice design
- Good work with git, a lot of commits per subject of work, etc.
- Very nice documentation with the readme file
- Requirements made well, core list functionality behaves as expected.
- Bonus additions function well, confetti and validation message are cool, local storage handling works good
- Code is a bit messy, a lot of code per function, some code duplication as well (see comments of CR), functions naming doesn't always align with what the function actually do
Best Practices:
- You should keep the same naming convention on element ids and classes : either kebab-case or CamelCase, here you mixed kebab-case and CamelCase in ids and classes
- Should wait for document ready event 'DOMContentLoaded' before fetching / attaching event listeners and querying elements
- Should not mix business logic and ui / visualization logic in the same functions. Keep function logic and separation of controls should to what the function is in charge of. (take a look on some of your functions:
addToDo- adds an item, play animation, saves to localstorage, and style the list
checkLocalStoragedoesn't just "check" as if expecting a boolean, it also get items and parse them (naming is confusing)
getTodos- doesn't just get the list it also create elements and add them to the DOM, changes the counter, styles the list and then might change some button to inactive (naming is confusing and also logic could be more separated) - Should not create elements by creating their string representation from JavaScript (`'')
- Code duplication - some chunks of code repeat themselves in your code, this is a great indicator to when you need to create more functions to insulate this functionality.
| const inputBox = document.querySelector(".inputField input"); | ||
| const svgContainer = document.getElementById("svg"); | ||
| const todoElement = document.getElementById("list-element"); | ||
| const footerSpan = document.querySelector(" .footer span "); |
There was a problem hiding this comment.
See comments above on footer, a better division of elements would have helped you here as well.
selecting a child element by type (span) of a parent by class (".footer") might cause bugs in the future if you'll add another span element, etc.
| todos.sort(); | ||
| todos.reverse(); |
There was a problem hiding this comment.
Why sort and then reverse? these are 2 consuming array functions.
sort function supports a compareFunc argument - see here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort
You can use the sort to both do ascending and descending
| const trashButton = document.createElement("button"); | ||
| trashButton.innerHTML = '<i class="fas fa-trash"></i>'; | ||
| trashButton.classList.add("trash-btn"); |
There was a problem hiding this comment.
this chunk of code is duplicated 3 times across the script - consider minimizing duplications and create functions for reoccurring functionality
| }, 1800); | ||
| } | ||
|
|
||
| //function to be used in later projects |
There was a problem hiding this comment.
It's bad practice to introduce code that doesn't have a purpose.
src/ex1/script.js
Outdated
| const todoInput = document.getElementById("input-txt"); | ||
| const todoButton = document.getElementById("add-btn"); | ||
| const todoList = document.getElementById("list-element"); | ||
| const clearAllBtn = document.getElementById("clearAll-btn"); | ||
| const sortBtn = document.getElementById("sort-btn"); | ||
| const inputBox = document.querySelector(".inputField input"); | ||
| const svgContainer = document.getElementById("svg"); | ||
| const todoElement = document.getElementById("list-element"); |
There was a problem hiding this comment.
All of the logic of getElement / querySelector should be after document ready event to ensure the above elements are actually exist on the DOM at the time of query
Almost ready...
Ori.s.Todo.list.-.Google.Chrome.2022-05-19.17-37-10.mp4