London | 25-ITP-May | Halyna Kozlovska | Sprint 2 | Book Library #257
Conversation
- add an omitted parenthesis
- fix the variable name of library to the correct one; - fix the name of the delete button
- change 'clicks' to 'click' to enable button functionality.
- show "Yes" when book is marked as read.
- Set correct <title> to "My Book Library" - Replace <div> with <form id="book-form"> for proper submission handling - Fix incorrect input types: "title" and "author" → "text" - Set checkbox value to "read" - Remove inline onClick handler from submit button - Clean up initial <tbody> structure to avoid empty table row
- Fix typo: "Robison Crusoe" → "Robinson Crusoe"
- Remove duplicate render() call after initial data push
- Replace inline form submission with `form.addEventListener('submit', ...)`
- Rename submit() → submitNewBook() for clarity
- Ensure pages are stored as a number in Book constructor
- Replace innerHTML with textContent for safer rendering
- Replace `==` with strict `===` for checkbox comparison
- Add `form.reset()` after successful book submission
halyna-k
added
📅 Sprint 3
halyna-k
changed the title
halyna-k
changed the title
halyna-k
added
📅 Sprint 2
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/cjyuan/Module-Data-Flows/blob/book-library-feedback/debugging/book-library/feedback.md
Doing so can help me speed up the review process. Thanks.
cjyuan
added
Reviewed
- replace constructor function with ES6 class for Book;
- rename variables ('check' → 'isRead') for semantic clarity;
- improve input validation logic: trims input, ensures page count is a valid number > 0;
- remove unnecessary else block after validation return;
- clean up event listener and DOM variable naming for better readability;
- reorganize button creation with modern const declarations;
- ensure proper UX: confirmation message shown after deletion, not before;
- use textContent consistently instead of innerHTML;
- remove unnecessary IDs and used button text instead;
- optimize table row clearing using a while loop.
@cjyuan Thank you for your comment. I've checked everything and corrected. |
| function submitNewBook() { | ||
| let title = titleInput.value.trim(); | ||
| let author = authorInput.value.trim(); | ||
| let pages = Number(pagesInput.value.trim()); |
There was a problem hiding this comment.
pagesInput could be a value like "3.1416". Do we want to allow such value as page count?
There was a problem hiding this comment.
@cjyuan Thanks for the great catch! I’ve updated the validation logic to require the page count to be a positive integer, preventing decimal values from being accepted.
debugging/book-library/script.js
Outdated
| while (table.rows.length > 1) { | ||
| table.deleteRow(1); | ||
| } |
There was a problem hiding this comment.
This approach is still deleting table rows one by one. A more efficient approach is to empty <tbody> in the table .
There was a problem hiding this comment.
@cjyuan Thanks for the suggestion! I updated the render function to clear the table body by setting its innerHTML to empty instead of deleting rows one by one. This should improve rendering efficiency, especially with larger datasets.
debugging/book-library/script.js
Outdated
| delBut.addEventListener("clicks", function () { | ||
| alert(`You've deleted title: ${myLibrary[i].title}`); | ||
| myLibrary.splice(i, 1); | ||
| const deleteCell = row.insertCell(4); |
There was a problem hiding this comment.
Can you think of the pros and cons of these two approaches for creating cells within a row?
- Keeping all the cell creation code in one location, like the original code does.
- Scattering the cell creation code across different locations, like what you did.
There was a problem hiding this comment.
@cjyuan Thank you for your comment.
Keeping all cell creation in one place improves readability and maintainability by clearly defining the row structure and minimizing the risk of mismatched cells. It also ensures consistent and efficient DOM updates.
Scattering cell creation allows for modular handling of complex UI elements or conditional logic but can fragment the code, making it harder to follow and maintain.
I refactored the code to centralize cell creation within render() for clarity, while extracting button-specific logic into helper functions to separate concerns without scattering cell insertion.
…etup - create all cells in one place within render(); - extract read toggle and delete button creation to helper functions; - improve code readability and maintainability
cjyuan
added
Complete
|
Changes look great. Well done! |
3 participants
Learners, PR Template
Self checklist
Changelist
Questions
Ask any questions you have for your reviewer.