London | 26-ITP-May | Russom Gebremeskel | Sprint 2 | Book Library - #562
London | 26-ITP-May | Russom Gebremeskel | Sprint 2 | Book Library #562russom-g wants to merge 40 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
cjyuan
left a comment
There was a problem hiding this comment.
Could consider placing all code that runs once on page load in a single function. For example, you could put it inside the page load callback or create a function named init() or setup() and call it once when the page loads.
This makes it easier to locate and manage all the code that runs once when the app starts.
|
Thanks for the feedback. Now a setup function is implemented. Both populateStorage and render function are wrapped inside the function. |
cjyuan
left a comment
There was a problem hiding this comment.
-
Could you remove
.vscode/settings.jsonand revert the changes made in.gitignore? Otherwise the validation bot will reject this PR. -
Could you also check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Flows/blob/general-review-feedback/debugging/book-library/feedback.mdDoing so can help me speed up the review process. Thanks.
|
Files that shouldn't be here are removed. I also have made some improvements following the requirement document. Thanks |
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good.
HTML code could use some more improvements. I will mark this PR as "Complete" first.
| <input | ||
| type="number" | ||
| class="form-control" | ||
| id="pages" | ||
| name="pages" | ||
| min="1" | ||
| required | ||
| /> | ||
| > |
There was a problem hiding this comment.
The browser checks the input elements against the specified constraints only when a user submits a form.
Without <form>, the browser won't check the input against the constraints such as required and min="1".
| <label class="form-check-label"> | ||
| <input | ||
| type="checkbox" | ||
| class="form-check-input" | ||
| id="check" | ||
| value="" | ||
| />Read | ||
| > |
There was a problem hiding this comment.
This checkbox is not showing.
The issue is related to Bootstrap 4.4.1. Could you use AI to find a way to fix the issue? Mentioning "Bootstrap 4.4.1" might help.
| this.author = author; | ||
| this.pages = pages; | ||
| this.check = check; | ||
| if (isNaN(pages) || pages <= 0) { |
There was a problem hiding this comment.
Could consider check if pages is a whole number instead.
|
note: You shouldn't have deleted the |
Learners, PR Template
Self checklist
Changelist
Bug fixed. Dead code removed.