Birmingham | ITP-Jan | Roman Sanaye | Sprint 2 | Book Library#436
Birmingham | ITP-Jan | Roman Sanaye | Sprint 2 | Book Library#436RomanSanaye wants to merge 3 commits intoCodeYourFuture:mainfrom
Conversation
RomanSanaye
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you 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.
cjyuan
added
Reviewed
|
Hi @cjyuan , I hope you well. |
RomanSanaye
added
the
Needs Review
| @@ -1,103 +1,122 @@ | |||
| let myLibrary = []; | |||
There was a problem hiding this comment.
Safer to declare it using const.
| // =====> DOM elements | ||
| const title = document.getElementById("title"); | ||
| const author = document.getElementById("author"); | ||
| const pages = document.getElementById("pages"); | ||
| const check = document.getElementById("check"); | ||
| const bookForm = document.getElementById("book-form"); |
There was a problem hiding this comment.
-
Could consider declaring these at the beginning of this file to keep all shared variables in one place.
-
Why not rename all these constants to better reflect they are DOM elements? (
bookFormis a good name)
| // =====> Form submit | ||
| bookForm.addEventListener("submit", function (e) { | ||
| e.preventDefault(); | ||
| submit(); | ||
| }); |
There was a problem hiding this comment.
Better to perform this in the window's on load event listener so that all code that needs to be executed once on page load in located in one place.
| alert(`Deleted: ${book.title}`); | ||
| myLibrary.splice(index, 1); | ||
| render(); |
There was a problem hiding this comment.
The alert message is shown before the book is actually deleted; the deletion only occurs after the alert dialog is dismissed. This introduces a risk that the operation may not complete (e.g., if the user closes the browser before dismissing the alert).
In general, it’s better to display a confirmation message only after the associated operation has successfully completed.
cjyuan
added
Complete
2 participants
Self checklist
Changelist
Fixed bugs in the Book Library application according to the assignment requirements.
The application now correctly:
Questions
No questions at this time.