Uh oh!
There was an error while loading. Please reload this page.
Glasgow | 26-ITP-May |Chan Yat Long |Sprint 2 | Book Library - #560
Glasgow | 26-ITP-May |Chan Yat Long |Sprint 2 | Book Library #560Motorcycle-lab wants to merge 9 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.
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.
Could you fix the checkboxes in the PR description?
Coud 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.mdDoing so can help me speed up the review process. Thanks.
cjyuan
commented
Aug 18, 2026
Did you push your changes to GitHub? |
cjyuan
commented
Aug 19, 2026
Could you go through the "general feedback" document again to see what else you could also improve? |
cjyuan
left a comment
There was a problem hiding this comment.
Have you also checked `index.html for errors or possible improvements?
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.
| alert("Are you sure you want to delete the book ?"); | ||
| myLibrary.splice(i, 1); | ||
| render(); |
There was a problem hiding this comment.
alert() won't let a user choose Yes or No. Besides, this message is not quite the same as the original message.
The intent of the original message is to inform the user which book was deleted.
| const table = document.getElementById("display"); | ||
| table.innerHTML = ""; | ||
| let rowsNumber = table.rows.length; | ||
| //delete old table | ||
| for (let n = rowsNumber - 1; n > 0; n-- { | ||
| table.deleteRow(n); | ||
| } | ||
| const tobody = table.querySelector("tbody") || table; | ||
| tobody.innerHTML = ""; |
There was a problem hiding this comment.
Why clear both
<table>and<tbody>?Where is
rowsNumberused?Where is
tobodyused?
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?
| <div id="demo" class="collapse"> | ||
| <form id="bookform"> |
There was a problem hiding this comment.
Good idea to introduce <form>. Why not use a formatter to keep the code consistently formatted?
| <input | ||
| type="number" | ||
| class="form-control" | ||
| id="pages" | ||
| name="pages" | ||
| required | ||
| /> |
There was a problem hiding this comment.
Why remove
required?Why not make this input element to accept only positive whole number?
| @@ -61,13 +59,9 @@ <h1>Library</h1> | |||
| value="" | |||
| />Read | |||
| </label> | |||
There was a problem hiding this comment.
Currently this checkbox is not showing.
The issue is related to Bootstrap 4.4.1. Could you use AI to find a way to fix the issue? Mentioning "Bootstrap 4.4.1" might help.
| class="btn btn-primary" | ||
| onclick="submit();" | ||
| /> | ||
| <button type="button" class="btn btn-primary mt-3" onclick="processbook()">Submit</button> |
There was a problem hiding this comment.
Could you look up the trade off between
- Assigning event listener in HTML
- Assigning event listener in JS using
.addEventListener()
| <input | ||
| type="submit" | ||
| value="Submit" | ||
| class="btn btn-primary" | ||
| onclick="submit();" | ||
| /> | ||
| <button type="button" class="btn btn-primary mt-3" onclick="processbook()">Submit</button> |
There was a problem hiding this comment.
Changing the submit button to a regular button means the browser won't check the constraints such as required when the user clicks the button. If your intention is to prevent the browser from submitting the form, which would cause the page to reload, you can call .preventDefault() on the event object passed to the event listener.
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good.
The issue with clearing the table should be easy to fix. I will mark this PR as "Complete" first.
| const table = document.getElementById("display"); | ||
| table.innerHTML = ""; |
There was a problem hiding this comment.
Clear the whole table would also remove the header rows. You could just clear the <tbody> of this table.
There was a problem hiding this comment.
Hello Mr cjyuan. You have approved me finally last night at 10pm, but I was not in the computer after that, so I cannot see the pull request which has been finished. I have been doing this pull request with you for four days before the deadline. I had finished every steps at that time and just had the book library left waiting you to get approved. I could not submit the deadline now because the time has passed. I am hoping that you can see my efforts that I had done this pull request with your for four days before the deadline.
There was a problem hiding this comment.
I am sorry but I was merely holding the same standard for every trainee.
You could still make it to the upcoming Checkpoint if you join the September ITP and and finished it within 1-2 weeks.
There was a problem hiding this comment.
but I will finish here because I am not avaliable for the upcoming ITP September. Is there anyway that I can stll receive the badge of this course
Self checklist
Changelist
The book project
Questions
If I can self learn all backend and frameworks by myself, build a porfolio by myself and become a full-stack engineer.