Cape Town | 2026-ITP-Jan | Pretty Taruvinga | Sprint 2 | Book Library#473
Cape Town | 2026-ITP-Jan | Pretty Taruvinga | Sprint 2 | Book Library#473Pretty548 wants to merge 5 commits intoCodeYourFuture:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
4 similar comments
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.
Pretty548
added
📅 Sprint 2
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
Pretty548
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Code looks good. I will mark this as "Complete" first.
| @@ -1,46 +1,45 @@ | |||
| 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.
Good point! I’ve updated myLibrary to use const to prevent reassignment.
| const titleInput = document.getElementById("title"); | ||
| const authorInput = document.getElementById("author"); | ||
| const pagesInput = document.getElementById("pages"); | ||
| const readCheckbox = document.getElementById("check"); |
There was a problem hiding this comment.
Normal practice is to declare all shared variables/constants at the beginning of the file (before function definition).
There was a problem hiding this comment.
Thanks for pointing that out , I’ll move these declarations to the top of the file for better organization.
| const table = document.getElementById("display"); | ||
|
|
||
| // remove old rows (keep header row) | ||
| while (table.rows.length > 1) { | ||
| table.deleteRow(1); | ||
| } |
There was a problem hiding this comment.
Note: Easier and more efficient to just clear <tbody>.
There was a problem hiding this comment.
Thanks for the suggestion — I’ve updated the render function to clear and rebuild the instead.
cjyuan
added
Complete
Co-authored-by: Copilot <copilot@github.com>
| const authorCell = document.createElement("td"); | ||
| const pagesCell = document.createElement("td"); | ||
| const wasReadCell = document.createElement("td"); | ||
| const deleteCell = document.createElement("td"); |
There was a problem hiding this comment.
Why not use the original approach? That is,
let titleCell = row.insertCell(0);
let authorCell = row.insertCell(1);
let pagesCell = row.insertCell(2);
let wasReadCell = row.insertCell(3);
let deleteCell = row.insertCell(4);There was a problem hiding this comment.
Thanks! I’ve applied this approach — I’m now using row.insertCell() for each column (title, author, pages, read status, delete), which simplifies the DOM manipulation and keeps the structure clear.
|
Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it. |
3 participants
Learners, PR Template
Self checklist
Changelist
Questions
None