London | 26-ITP-May | Bisrat Tesfay | Sprint 2 | Book-Library - #536
London | 26-ITP-May | Bisrat Tesfay | Sprint 2 | Book-Library#536B3-Bisrat wants to merge 2 commits into
Conversation
B3-Bisrat
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can 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.md
Doing so can help me speed up the review process. Thanks.
There was a problem hiding this comment.
According to https://validator.w3.org/, there are errors in your index.html. Could you fix these errors?
There was a problem hiding this comment.
-
Even though this element has a
requiredattribute, the user could still "submit" the input the even if this input field is empty. Could you find out why and then address the issue? -
For better user experience, you could also consider making this input field to accept only positive whole numbers.
There was a problem hiding this comment.
The 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.
There was a problem hiding this comment.
Can we declare myLibrary in a way that prevents it from being accidentally reassigned?
| value="Submit" | ||
| class="btn btn-primary" | ||
| onclick="submit();" | ||
| onclick="addBook()" |
There was a problem hiding this comment.
Could you research the trade-off between
- Attaching event listener in HTML
- Attaching event listener using
addEventListener()in JS
cjyuan
added
Reviewed
B3-Bisrat
added
the
Needs Review
|
Thank you for the feedback and comments. I’ve made the suggested changes based on your feedback. Could you please have a look and let me know if there’s anything else I should improve? |
B3-Bisrat
removed
the
Reviewed
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look great. Well done.
| required | ||
| /> | ||
| <label class="form-check-label"> | ||
| <form id="bookForm"> |
There was a problem hiding this comment.
Could use lower kebab case for a naming consistency.
| myLibrary.splice(i, 1); | ||
| render(); | ||
|
|
||
| alert(`You've deleted title: ${deletedTitle}`); |
There was a problem hiding this comment.
alert() is a blocking function call. As a result, invoking it prevents the browser from updating the UI until the dialog is dismissed.
If time permits, research for approaches that allows the UI to update before displaying the alert dialog. (This is an optional change).
| const checkInput = document.getElementById("check"); | ||
| const bookForm = document.getElementById("bookForm"); | ||
|
|
||
| bookForm.addEventListener("submit", addBook); |
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.
cjyuan
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
Fixed the library bugs, including adding, displaying, reading status, and deleting books. Also fixed form validation and author input issues.
Questions