-
-
Notifications
You must be signed in to change notification settings - Fork 69
LON | Amir Montazeri | Module-Data-Flows | Book-Library #165
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
base: main
Are you sure you want to change the base?
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.
Good job fixing the bugs - the application is working well now.
You also did a good job replacing the 'let' variables with 'const'. 👏
Please make the following changes to make your code even better.
-
The browser tab currently shows '127.0.0.1:555/debugging/book-library. Can you update it to be more explanatory to the user. If you want to read more about this here is a doc: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/title
-
The changelist portion of your pull request is currently empty. This is an important part that helps other engineers understand what you've modified.
Could you please add a brief description of your changes in the changelist section? Even a simple summary is will be beneficial.
debugging/book-library/script.js
Outdated
|
||
// Read/Unread Toggle Button | ||
const changeBut = document.createElement("button"); |
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.
It's possible that using changeBut to denote a button might be confusing to someone reviewing your code. Can you replace with a clearer name here and at line 86? Here are some tips:
- it's recommended to start with a verb for functions
- it's okay to use btn to shorten button. Personally, I use 'button' to be clear
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.
@jenny-alexander thank you for review, I changed the name of variable to a more descriptive name.
Learners, PR Template
Self checklist
Changelist
Questions
Ask any questions you have for your reviewer.