-
-
Notifications
You must be signed in to change notification settings - Fork 41
NW6 | Rabia Avci | JS2 Module | [TECH ED] Build todo-list app | Week 4 #222
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. |
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.
Good Implementation overall, I like the background touch.
- Your TODO doesnt have any deadline.
- The done icon and the trash icon can be improved with better spacing
|
||
function addItem(task) { | ||
if (!task){ | ||
alert("To-do is required!"); |
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.
good catch here
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.
I think this looks great and works really well. Well done!
</div> | ||
</form> | ||
</section> | ||
</container> |
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.
I love your use of semantic HTML and the consistant naming conventions 👍
|
||
function addItem(task) { | ||
if (!task){ | ||
alert("To-do is required!"); |
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.
👍
const classList = item.classList; | ||
classList.contains("done") | ||
? classList.remove("done") | ||
: classList.add("done"); |
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.
Could use .toggle() here if you wanted next time - it's a helpful function to save writing extra code https://stackoverflow.com/questions/18880890/how-do-i-toggle-an-elements-class-in-pure-javascript
const doneItems = list.querySelectorAll(".done"); | ||
for (const item of doneItems) { | ||
list.removeChild(item); | ||
} |
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.
All in all, some solid functional code here. It could probaly benefit from a refactor and some comments but we're not exspecting perfection here - it works that's the main thing. Good work!
|
||
body { | ||
background-image: url(assets/pattern_squares-2_1_4_0-0_180_2__ee6d4d_293241_e0fbfc_3d5a80_98c1d9.png); |
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.
Loving the background image!
flex-direction: column; | ||
justify-content: center; | ||
align-items: center; | ||
} |
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.
Good use of flex 👍
<button | ||
type="button" | ||
id="remove-all-completed" | ||
class="btn btn-primary mb-3" |
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.
Doesn't look like you're using these class names in the CSS. They look like classes from Bootstrap CSS. When copy pasting always make sure you tidy up the code so that you can be certain to understand why it works :)
color: white; | ||
padding: 10px 25px; | ||
text-align: center; | ||
} |
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.
Some really nice styles, consistant theming etc, no issues I can see except that the delete and completed buttons might be a bit nicer with a bit of spacing
<title>Todo List</title> | ||
<link | ||
rel="stylesheet" | ||
href="https://maxcdn.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.min.css" |
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.
I like how you've included font awesome for the icons - icons are always a nice touch!
Learners, PR Template
Self checklist
Changelist
Completed a to-do list app populating a "to-do list" element with tasks and associated completion buttons. Have functions to add new tasks, mark tasks as completed, and delete completed tasks.
Questions
Ask any questions you have for your reviewer.