London | 26-ITP-Jan | Alex Okorefe | Sprint 2 | Book Library#454
London | 26-ITP-Jan | Alex Okorefe | Sprint 2 | Book Library#454Alex-Os-Dev-Lab wants to merge 4 commits intoCodeYourFuture:mainfrom
Conversation
Alex-Os-Dev-Lab
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
Alex-Os-Dev-Lab
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. Can you fix these errors? -
Not all bugs have been fixed.
-
There are quite some improvements you can still make, and I only highlighted some.
Please check this general feedback to find out how else you can further improve your code :
https://github.com/CodeYourFuture/Module-Data-Flows/blob/general-review-feedback/debugging/book-library/feedback.md
| @@ -1,6 +1,6 @@ | |||
| let myLibrary = []; | |||
There was a problem hiding this comment.
Can we declare myLibrary in a way that prevents it from being accidentally reassigned?
There was a problem hiding this comment.
Can you address this comment?
cjyuan
added
Reviewed
…ed and reset correctly
Alex-Os-Dev-Lab
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. Can you fix these errors?
| @@ -66,17 +86,17 @@ function render() { | |||
| let pagesCell = row.insertCell(2); | |||
| let wasReadCell = row.insertCell(3); | |||
| let deleteCell = row.insertCell(4); | |||
There was a problem hiding this comment.
These variables are also not going to be reassigned.
| delBut.className = "btn btn-warning"; | ||
| delBut.innerHTML = "Delete"; | ||
| delBut.addEventListener("clicks", function () { | ||
| delButton.id = i; |
There was a problem hiding this comment.
- Lines 95, 113:
idattribute should be unique. Are the values assigned to theseidattributes unique?- Is there any need to assign an id attribute to either buttons?
There was a problem hiding this comment.
This could be a good opportunity to practice using the ? : conditional operator. Can you rewrite the code on lines 98–104 as a single statement?
| alert(`You've deleted title: ${myLibrary[i].title}`); | ||
| myLibrary.splice(i, 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.
| let table = document.getElementById("display"); | ||
| let rowsNumber = table.rows.length; | ||
| //delete old table | ||
| for (let n = rowsNumber - 1; n > 0; n-- { | ||
| for (let n = rowsNumber - 1; n > 0; n--) { | ||
| table.deleteRow(n); | ||
| } |
There was a problem hiding this comment.
It is easier and more efficient just to clear <tbody>; no need to use loop.
cjyuan
added
Reviewed
…rm submission and rendering logic Co-authored-by: Copilot <copilot@github.com>
|
Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it. |
3 participants
…Book object
Learners, PR Template
Self checklist
Changelist
Completed tasks related to the debugging/book-library repo
Questions
None at this stage