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

Second phase completed #6

Merged
merged 1 commit into from
Dec 8, 2021
Merged

Second phase completed #6

merged 1 commit into from
Dec 8, 2021

Conversation

Hombre2014
Copy link
Owner

All the requirements from the step 2 - interactive list, have been implemented:

  • Add a new JavaScript files and import it as a module:
    • it will contain methods related to the status updates (completed: true / false).
  • Add event listener to the checkbox (change).
  • Update items object's value for completed key upon user actions.
  • Store the updated array of items in local storage, so the user gets the correct list values after the page reloads.

Copy link

@mikenath223 mikenath223 left a comment

Choose a reason for hiding this comment

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

Hello @Hombre2014

You've done pretty well so far!
But for a few issues, you are almost done with this. Hang in there!

Required Changes ♻️

Please check the comments under the review.

Happy coding, Cheers! 🚀🚀

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

Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, whether it is 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.

@@ -65,7 +73,7 @@ To get a local copy up and running follow these simple example steps.

### Deployment

- All the files necessary for deployment are in the /dist folder
- All the files necessary for deployment are in the `/dist` folder

Choose a reason for hiding this comment

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

  • In src/status.js kindly change all regular functions to es6 arrow function syntax following the JavaScript best practices guide.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I really do not understand your first comment about the README file. I do have

  • All the files necessary for deployment are in the /dist folder stated there! Please, clarify!
    For your second comment I would say:
  • I used arrow functions where I thought is appropriate and useful. For example in checkButton function. There is no specific requirements to have all my regular functions to be arrow functions! Please, have a look at project requirements from my README.md file (step 2).
  • In Java Best practices article from Microverse there is no rule to have all regular functions as arrow functions.
  • In code clean article cited above also there is nothing stating that you should use only arrow functions!
  • I am repeating the week and this same branch (code) have been approved last week. You can see it here.
    So, I do not really understand your rejection. These might be recommendations and optional changes, but rejection? Please, reply.

Copy link

@mikethreels mikethreels left a comment

Choose a reason for hiding this comment

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

Hi Yuriy ,

Apologies for the misunderstanding in the previous review, usage of ES6 arrow functions is indeed not a requirement.

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

Cheers and Happy coding!👏👏👏

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.

@Hombre2014 Hombre2014 merged commit 44af24a into main Dec 8, 2021
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.

3 participants