Glasgow | 26-ITP-May | Sandani Kannangara | Sprint 2 | Fix library debugging - #524
Glasgow | 26-ITP-May | Sandani Kannangara | Sprint 2 | Fix library debugging#524SandzSoft wants to merge 7 commits into
Conversation
SandzSoft
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
SandzSoft
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
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?
| @@ -1,103 +1,109 @@ | |||
| let myLibrary = []; | |||
There was a problem hiding this comment.
Could 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 isReadInput = document.getElementById("isRead"); | ||
| const bookForm = document.getElementById("book-form"); |
There was a problem hiding this comment.
Common practice is to declare all variables before function definitions.
| bookForm.addEventListener("submit", function (event) { | ||
| event.preventDefault(); | ||
| addBook(); | ||
| }); |
There was a problem hiding this comment.
Why not place all "code that runs once on page load" in one place. For example, inside the on page load callback function.
| titleInput.value = ""; | ||
| authorInput.value = ""; | ||
| pagesInput.value = ""; | ||
| isReadInput.checked = false; |
There was a problem hiding this comment.
You could also use the form's built-in .reset() method to reset the form.
| 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
Reviewed
2 participants
Self checklist
Changelist