Skip to content
This repository has been archived by the owner on Dec 18, 2024. It is now read-only.

NW6 | Rabia Avci | JS2 Module | [TECH ED] Build todo-list app | Week 4 #222

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

RbAvci
Copy link

@RbAvci RbAvci commented Jan 24, 2024

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

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.

Copy link

netlify bot commented Jan 24, 2024

Deploy Preview for cute-gaufre-e4b4e5 ready!

Name Link
🔨 Latest commit 5eef4bf
🔍 Latest deploy log https://app.netlify.com/sites/cute-gaufre-e4b4e5/deploys/65b37a3c0e988500084780fc
😎 Deploy Preview https://deploy-preview-222--cute-gaufre-e4b4e5.netlify.app/week-3/todo-list
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@bunday bunday left a 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!");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch here

Copy link

@Ara225 Ara225 left a 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>
Copy link

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!");
Copy link

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");
Copy link

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);
}
Copy link

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);
Copy link

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;
}
Copy link

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"
Copy link

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;
}
Copy link

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"
Copy link

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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants