London | 26-ITP-May | Zadri Abdule | Sprint 2 | Book - library - #564
London | 26-ITP-May | Zadri Abdule | Sprint 2 | Book - library#564Zadri415 wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
Zadri415
added
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
Zadri415
changed the title
Zadri415
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
-
To make it clearer to reviewers that the code has already been reviewed in a separate PR, you could consider including the following in the PR description:
- A link to the previous PR.
- An explanation of why the previous PR was closed.
-
Could you address the comments I left on the previous PR, in addition to the new comments?
| <input | ||
| type="number" | ||
| class="form-control" | ||
| id="pages" | ||
| name="pages" | ||
| required | ||
| /> |
There was a problem hiding this comment.
-
The
"required"attribute is checked by the browser when a user submits a form, but currently the input elements are not in a form. -
In the previous PR, you had set the attributes to ensure this element accepts only positive whole integers. Why left the attributes out?
| <input | ||
| type="checkbox" | ||
| class="form-check-input" | ||
| id="check" | ||
| value="" | ||
| />Read |
There was a problem hiding this comment.
If this checkbox is not showing, could you try fixing the issue with the help of AI. The issue is related to Bootstrap 4.4.1. So mentioning "Bootstrap 4.4.1" to AI might help.
| value="Submit" | ||
| class="btn btn-primary" | ||
| onclick="submit();" | ||
| onclick="addBook()" |
There was a problem hiding this comment.
Suggestion: Look up the trade-off between
- assigning event listener in HTML
- assigning event listener in JS via
.addEventListener()
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
index.html — fix form input types and wiring (changed submit button to call addBook()).
script.js — fixes for seeding, add-book handling, read state (use boolean), rendering, toggle read, and delete logic.
Questions
N/A