Uh oh!
There was an error while loading. Please reload this page.
London | 26-ITP-May | Mandip Sanger | Sprint 2 | Book Library - #569
London | 26-ITP-May | Mandip Sanger | Sprint 2 | Book Library#569mandipsanger wants to merge 10 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
mandipsanger
commented
Aug 21, 2026
My Program is working now but failing checks Validate PR Metadata workflow run. |
mandipsanger
commented
Aug 21, 2026
its showing Validate PR Metadata thread 'main' (2352) panicked at src/bin/pr-metadata-validator.rs:74:6: |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| @@ -25,22 +21,44 @@ const author = document.getElementById("author"); | |||
| const pages = document.getElementById("pages"); | |||
| const check = document.getElementById("check"); | |||
There was a problem hiding this comment.
Could you name these variables to emphasise that they are input elements?
Uh oh!
There was an error while loading. Please reload this page.
| for (let n = rowsNumber - 1; n > 0; n--) { | ||
| table.deleteRow(n); | ||
| } |
There was a problem hiding this comment.
Could you look up a more efficient approach (than deleting table rows one by one) to clear the <tbody> part of a table?
| 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.
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
commented
Aug 21, 2026
If you have made any changes, you need to push the commits to GitHub. |
mandipsanger
commented
Aug 21, 2026
i tried my best. I think i am tired now |
| const pageNumber = Number(pagesValue); | ||
| // Check that pages is greater than zero | ||
| if (pageNumber <= 0) { |
There was a problem hiding this comment.
Note: Could also consider using Number.isInteger() on line 54 instead of using regular expression on line 45.
Self cheklist
Sprint 2 Data flows book library