London | 26-ITP-May | Martin Mwaka | Sprint 2 | Book library - #523
London | 26-ITP-May | Martin Mwaka | Sprint 2 | Book library#523Temceo wants to merge 6 commits into
Conversation
…dd console 4. correct was read message
…submit as this should be done in JavaScript
…Button variable and eventListener
…js - update to accept populate table from fragment and refactor to make content easier to understand - style.css - add css for delete book message
This comment has been minimized.
This comment has been minimized.
Temceo
added
🏕 Priority Mandatory
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.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Temceo
added
the
Needs Review
cjyuan
left a comment
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.
cjyuan
added
Reviewed
|
I have reviewed the general feedback and improved the code |
Temceo
added
Needs Review
| <label for="pages">Pages:</label> | ||
| <input type="number" class="form-control" id="pages" name="pages"> | ||
| <span class="error error-pages"></span> |
There was a problem hiding this comment.
Why not make this input element reject unwanted input values?
| const displayBooks = document.querySelector(".display-books"); | ||
| const bookEntryTab = document.getElementById("demo"); | ||
|
|
||
| let myLibrary = []; |
There was a problem hiding this comment.
Can we declare myLibrary in a way that prevents it from being accidentally reassigned?
| myLibrary.push(new Book("Robison Crusoe", "Daniel Defoe", "252", true)); | ||
| myLibrary.push( | ||
| new Book("The Old Man and the Sea", "Ernest Hemingway", "127", true) | ||
| ); |
There was a problem hiding this comment.
Why represent page count as a string?
| addBookForm.addEventListener("submit", (event) => { | ||
| event.preventDefault(); | ||
| processEntries(); | ||
| }); |
There was a problem hiding this comment.
To make locating all the "code that is to be executed once on page load" easier, a common practice is to keep the code in the same place. For example, inside the page's onload callback function.
| }); | ||
|
|
||
| if (isValid) { | ||
| let book = new Book(title.value, author.value, pages.value, check.checked); |
There was a problem hiding this comment.
-
The raw values used here could be different from those checked on line 75.
-
pages.valuecould be a value like"3e2"or"12.345". Should page count be assigned values like these?
| const table = document.createElement("table"); | ||
| table.className = "table table-striped"; | ||
|
|
||
| const thead = document.createElement("thead"); | ||
| const headerRow = document.createElement("tr"); | ||
| ["Title", "Author", "Pages", "Read", "Delete"].forEach((label) => { | ||
| const th = document.createElement("th"); | ||
| th.textContent = label; | ||
| headerRow.appendChild(th); | ||
| }); | ||
| thead.appendChild(headerRow); | ||
| table.appendChild(thead); |
There was a problem hiding this comment.
Why dynamically create the static table its header rows every time render() is called? Why not keep them in index.html as HTML code?
cjyuan
added
Reviewed
2 participants
Self checklist
Changelist
Debug and improve functionality of the library app