Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Core Functionality With Some Extras #8

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

yuriyivanenko
Copy link

To-Do app is able to create, and delete tasks. Also empty input field will not result in creating blank tasks.

Copy link

@stephenmckeon stephenmckeon left a comment

Choose a reason for hiding this comment

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

This is awesome! Nice job. Next challenge for you: How can you refactor this to make it more readable? Is there any duplication that can be removed? Can you rename things to make them more readable? Try to get each function down to 10 lines at most for extra fun.

Copy link

@stephenmckeon stephenmckeon left a comment

Choose a reason for hiding this comment

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

You're crushing it! My previous comment still applies, but I love the extras. How about including a few screenshots or even a video of the functionality in the PR description? Maybe improve the PR write up too? All just extras, but would be good experience.

@@ -4,24 +4,34 @@
<meta charset="UTF-8">
<title>Flatiron Task Lister</title>
<link rel="stylesheet" href="./style.css">
<script defer src="./src/index.js"></script>

Choose a reason for hiding this comment

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

What does the defer attribute do?

Copy link
Author

Choose a reason for hiding this comment

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

It defers loading the script to after the DOM has loaded. I like placing the script tag up top.

@yuriyivanenko
Copy link
Author

Screen.Recording.2024-05-16.at.2.57.51.PM.mov

Video of current functionality. Needs more code refactoring to slim down functions.

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

Successfully merging this pull request may close these issues.

2 participants