London | 25-ITP-May | Houssam Lahlah | Sprint 2 | feature/book library #313
Conversation
- change library.push(book) to myLibrary.push(book). - add a ) to the end of the for loop condition to avoid syntax error. - change myLibrary[i].check to myLibrary[i].check == true to check if the book is read or not. - change delBut to delButton because we declare it. - The event listener is "clicks" instead of "click".
HoussamLh
added
the
Needs Review
sambabib
added
Review in progress
| let book = new Book(title.value, title.value, pages.value, check.checked); | ||
| library.push(book); | ||
| //fix: change title.value to author.value | ||
| let book = new Book(title.value, author.value, pages.value, check.checked); |
| for (let n = rowsNumber - 1; n > 0; n-- { | ||
| // Fix : add a ) to the end of the for loop condition | ||
| // to avoid syntax error. | ||
| for (let n = rowsNumber - 1; n > 0; n--) { |
There was a problem hiding this comment.
Good catch of the syntax error here as well.
| //add and wait for action for read/unread button | ||
| let changeBut = document.createElement("button"); | ||
| changeBut.id = i; | ||
| changeBut.className = "btn btn-success"; |
There was a problem hiding this comment.
Whilst this isn’t a bad variable name, it is good practice to have clear variable names so the purpose can be inferred at a quick glance. For example, changeButton would have been better here. Something to note moving forward.
| // Fix: change myLibrary[i].check to myLibrary[i].check == true | ||
| // to check if the book is read or not. | ||
| if (myLibrary[i].check == true) { | ||
| readStatus = "Yes"; |
There was a problem hiding this comment.
When you build applications with status updates, it is good practice to store these statuses in variables/objects. For a small application like this, it is not necessary but as your codebase grows bigger, it becomes easier to slot in the status variable than constantly type in a string (typos could happen). This is something to note.
| delBut.className = "btn btn-warning"; | ||
| delBut.innerHTML = "Delete"; | ||
| delBut.addEventListener("clicks", function () { | ||
| delButton.id = i + 5; |
There was a problem hiding this comment.
See. You used a better variable name here, so try to keep it consistent.
sambabib
added
Complete
3 participants
Self checklist
## Changelist
Fixed issue where books did not display on page load
Corrected bug where title was being used as author when adding new books
Fixed delete button (wrong variable name + wrong event type)
Corrected read/unread toggle so it shows the correct value
Cleaned up table rendering loop to properly remove old rows
Minor syntax corrections (missing bracket in for loop)
## Questions
Is my render() function written in a clean way, or should I break it into smaller functions?
Would you recommend converting the Book constructor into a class for better readability?