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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Peer-to-peer code review 12/07/2022 #5

Closed
1 task done
lucas-crodrigues opened this issue Jul 12, 2022 · 1 comment
Closed
1 task done

Peer-to-peer code review 12/07/2022 #5

lucas-crodrigues opened this issue Jul 12, 2022 · 1 comment

Comments

@lucas-crodrigues
Copy link

lucas-crodrigues commented Jul 12, 2022

First of all, congratulations on your code, the app looks great, and there are no known bugs 馃敟 馃挴

Here are a few suggestions to improve on it:
https://github.com/wdavidcch/to-do-list/blob/d0a04d0ef2104c40ff87edf019b4b0077f147171/src/index.js#L29-L39

  • Here if you put the input in a form with a button, both with "type = 'submit' " you can just have an event listener for "submit", it'll grab both click and keypress, the way it is right now you don't need the preventDefault since it's not a form
@jaferIdrees
Copy link

jaferIdrees commented Jul 12, 2022

  • For the page functionality I noticed that when you change any task completed status by checking the checkbox, it doesn't update the value in the localStorage hence if you refresh the page you lose these changes, I believe you need to import the checkbox event listener from the module and use it in index.js. 馃憤

JavaScript best practices

  • it's recommended to keep all variables declarations on the top of your code. 馃崚

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

No branches or pull requests

3 participants