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
To Do list: list structure #1
Conversation
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.
Hi @RitobrotoMukherjee,
Good job so far!
There are some issues that you still need to work on to go to the next project but you are almost there! Highlights for your project:
- Followed git workflow
- No linter errors
Required Changes ♻️
Check the comments under the review.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
_As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form
src/index.js
Outdated
template += `<li class="todo-list-items"> | ||
<div class="list-item-data"> | ||
<input type="checkbox" id="task${item.index}" name="checkbox" value="${item.index}"> | ||
<label for="task${item.index}">Task ${item.index + 1}</label> | ||
</div> | ||
<div class="list-item-data"> | ||
<i id="delete${item.index}" class="fas fa-trash-alt"></i> | ||
</div> | ||
</li>`; | ||
}); |
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 suggest that you add your simple to-do list in an array(array of objects) and each task should have 3 keys
description
,completed
, andindex
instead of adding them like HTML elements
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.
- Please work on the function that iterates over the
tasks
array and populates the HTML list item element for each task,tasks
should be rendered dynamically to the DOM, and thetasks
should appear in order of theindex
values for each task.
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.
Hi, @DeliceLydia I think you might have overlooked the architecture of the code.
All tasks are object of task.js
module and the array list is getting dynamically implemented from taskList.js
module's getList()
function. It's a dynamic JS list getting rendered to HTML list. It's an Array of object from task.js
file.
For more clarification please look into item.index
, that index
is in proper order and dynamically generated, even that is accessed as dot object accessor of JS. I have followed proper OOP
and JS modular structure here.
Please consider going through the full code of task.js
and taskList.js
file and line number 5 and 10 from index.js
to understand the code flow properly.
If after following the steps you have any confusion understanding the flow please drop a comment.
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 suggest that you add your simple to-do list in an array(array of objects) and each task should have 3 keys
description
,completed
, andindex
instead of adding them like HTML elements
The Array is the taskList
getting generated using task.js
default object having the keys index
, completed
, description
. Please consider checking the code flow. I have followed proper OOP
with JS modular architecture. So, the changes you have requested are already implemented.
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.
- Please work on the function that iterates over the
tasks
array and populates the HTML list item element for each task,tasks
should be rendered dynamically to the DOM, and thetasks
should appear in order of theindex
values for each task.
Hi @DeliceLydia ,The forEach
loop on taskList
array is rendering everything dynamically on HTML DOM, in proper order of index starting from 0
index till 2
, as I am rendering 3 elements. Please consider rechecking the code flow. What you have requested for is already implemented.
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.
Hello @RitobrotoMukherjee, Thank you for reaching out! I get what you mean only just it was suggested for this milestone to add manually some simple tasks in the array then render them dynamically to the DOM just like:
const tasks = [
{
description: 'An example description in string',
completed: false,
index: 10,
},
];
Ask another review and it will get approved you can add extra information while submitting
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.
Hi, @DeliceLydia Thanks for replying to me back. Sure, I will refactor the code with just a static array and resubmit it again without importing the modules as you suggested!
In my advocate: I took this approach as I thought this will follow SOLID and DRY principles both. Other than that I saw the sneak peak
and found this structure will help me complete the rest of the tasks on this project faster being the foundation of the project is strong.
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.
Hi @RitobrotoMukherjee,
Status: Approved ✔️
Excellent job on making the changes requested by the previous code reviewer. Keep it up 👏
Your project is complete! There is nothing else to say other than... it's time to merge it 💪
Congratulations! 🎉
[Optional] suggestions:
- Nothing to mention.
Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag @topeogunleye in your question so I can receive the notification.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
Review Changes: review 1
task list
li
elements from the static array of the task objects.OOP
related code for the time being, as these codes will be helpful in coming tasks for the same project.Set up Todo List Skeleton with Webpack
TaskList module