London | 26-ITP-May | Chinwe Chukwuma | Sprint 2 | Book-Library - #526
London | 26-ITP-May | Chinwe Chukwuma | Sprint 2 | Book-Library#526ChinweP wants to merge 7 commits into
Conversation
ChinweP
added
📅 Sprint 2
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, thanks for your feedback. I've gone through the feedback and made several improvements to my Book Library. I've also tested the changes to make sure everything is still working correctly. Thanks again for your guidance. |
ChinweP
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good. Well done.
| @@ -1,103 +1,114 @@ | |||
| let myLibrary = []; | |||
There was a problem hiding this comment.
Can we declare myLibrary in a way that prevents it from being accidentally reassigned?
| const titleInput = document.getElementById("title"); | ||
| const authorInput = document.getElementById("author"); | ||
| const pagesInput = document.getElementById("pages"); | ||
| const checkInput = document.getElementById("check"); | ||
| const bookForm = document.getElementById("book-form"); |
There was a problem hiding this comment.
Common practice is keep all variable declarations at the beginning of the file (before function definition).
| if (!Number.isInteger(pageCount) || pageCount < 1) { | ||
| alert("Pages must be a positive whole number!"); | ||
| return false; | ||
| } | ||
| const book = new Book(titleInput.value.trim(), authorInput.value.trim(), pageCount, checkInput.checked); |
| titleInput.value = ""; | ||
| authorInput.value = ""; | ||
| pagesInput.value = ""; | ||
| checkInput.checked = false; |
There was a problem hiding this comment.
Could also call the .reset() method of the form element.
| const deletedTitle = myLibrary[i].title; | ||
| 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).
cjyuan
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
Book library coursework completed following readme instructions.