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

to-do List Milestone 3 : Interactive list #4

Merged
merged 7 commits into from Feb 19, 2022
Merged

Conversation

Nemwel-Boniface
Copy link
Owner

@Nemwel-Boniface Nemwel-Boniface commented Feb 18, 2022

In this milestone I was able to :

  • Practice use of Javascript modules
  • Practice Javascript DOM manipulation
  • Practice use of ES6 syntax
  • Finish all CRUD functionality for my to-do list application

Known Issue

  • sometimes when you delete the checked tasks and add new ones and delete them again the delete functionality "hangs". Delete directly from local storage and refresh the page and everything should be fine.

@github-pages github-pages bot temporarily deployed to github-pages February 18, 2022 18:52 Inactive
Copy link

@Ghiftee Ghiftee left a comment

Choose a reason for hiding this comment

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

STATUS: CHANGES REQUIRED ♻️♻️

Hello 👋🏽 @Nemwel-Boniface
Good job working on the project so far!
✔️ No linter errors.
✔️ App runs smoothly.
✔️ Tasks are updated correctly.
However, there are some issues that you need to work on to go to the next project but you are almost there!

Required Changes ♻️

Check the comments under the review.

Optional suggestions

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.

  • Since having a working live link is not a requirement in this project. I would only suggest that you consider updating your live link as it currently gives a 404 error.

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.

@@ -0,0 +1,13 @@
import { addToLocalStorage, tasks } from '../index.js'; //eslint-disable-line
Copy link

@Ghiftee Ghiftee Feb 18, 2022

Choose a reason for hiding this comment

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

  • Linters are helpful in ensuring that we have clean code and use the best practices so kindly remove this disable and please try to fix the error instead.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hello @Ghiftee thank you for this review.
I tried resolving this before and I was not successful. Might you be having a solution for this/ a configuration I should make .

Copy link

Choose a reason for hiding this comment

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

Hi, I currently do not have a solution but I am looking for one. However, It might take a while because I am a bit preoccupied at the moment. I will let you know when I have found a solution if you haven't by then.

Copy link

Choose a reason for hiding this comment

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

Hi @Nemwel-Boniface can you please check your slack dm

Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay

src/index.js Outdated
tasks[i].index = tasks[i].index -= 1; //eslint-disable-line
}
addToLocalStorage();
displayTasks(); //eslint-disable-line
Copy link

@Ghiftee Ghiftee Feb 18, 2022

Choose a reason for hiding this comment

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

  • Linters are helpful in ensuring that we have clean code and use the best practices so kindly remove this disable and please try to fix the error instead.

src/index.js Outdated
@@ -32,6 +34,23 @@ const editTask = (desc, index) => {
addToLocalStorage();
};

const resetIndex = () => {
for (let i = 0; i < tasks.length; i += 1) {
tasks[i].index = tasks[i].index -= 1; //eslint-disable-line
Copy link

@Ghiftee Ghiftee Feb 18, 2022

Choose a reason for hiding this comment

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

  • Linters are helpful in ensuring that we have clean code and use the best practices so kindly remove this disable and please try to fix the error instead.

src/index.js Outdated
@@ -1,12 +1,14 @@
import _ from 'lodash'; //eslint-disable-line
import './style.css';
import setState from './modules/getstates.js'; //eslint-disable-line
Copy link

@Ghiftee Ghiftee Feb 18, 2022

Choose a reason for hiding this comment

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

  • Linters are helpful in ensuring that we have clean code and use the best practices so kindly remove this disable and please try to fix the error instead.

src/index.js Outdated
@@ -1,12 +1,14 @@
import _ from 'lodash'; //eslint-disable-line
Copy link

@Ghiftee Ghiftee Feb 18, 2022

Choose a reason for hiding this comment

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

  • Linters are helpful in ensuring that we have clean code and use the best practices so kindly remove this disable and please try to fix the error instead.

@github-pages github-pages bot temporarily deployed to github-pages February 18, 2022 20:47 Inactive
Copy link

@Hamzaoutdoors Hamzaoutdoors left a comment

Choose a reason for hiding this comment

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

Hi @Nemwel-Boniface,

Congratulations! 🎉 Project Approved ✅

Your project is complete! There is nothing else to say other than... it's time to merge it 🚀

To Highlight:

  • The Pull Request has a proper title and description. ✔️
  • You have added a descriptive Readme file. Good Job. ✔️
  • There is no linter error is present. ✔️
  • Project has met all the technical aspects you needed for this project! ✔️

Cheers and Happy coding and Keep rocking! 👏👏👏

Feel free to leave any questions or comments in the PR thread if something is not 100% clear.


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.

@Nemwel-Boniface Nemwel-Boniface merged commit 4e53d3d into main Feb 19, 2022
@Nemwel-Boniface Nemwel-Boniface deleted the interactiveList branch February 19, 2022 04:08
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.

None yet

3 participants