-
-
Notifications
You must be signed in to change notification settings - Fork 41
NW6 | Sabella Fisseha | JS2 | To Do List | WEEK 3 #214
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for cute-gaufre-e4b4e5 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
<link rel="stylesheet" href="style.css" /> | ||
<link |
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.
In the end I don't think you used a custom font, so this <link>
tag can be removed. We want to make sure that the site loads quickly for users and doesn't download unnecessary files, so removing this is an easy performance win! 💯
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.
In this file, there's two large chunks of code that are very similar, inside the populateTodoList
and addNewTodo
functions. Generally, we don't want to re-use code (a concept called DRY - Don't Repeat Yourself). Repeating code can make it difficult to make a change in the future - it's nice to only have to update code in one place
As a starting point - maybe some of the repeated code could be extracted into its own function? Both seem to be creating a new todo element, so maybe a function called createTodo
or similar would be a good place for the code
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.
Wow! This is great work Sabella! You've hit all the requirements and completed the advanced challenge!
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.