London | 26-ITP-Jan | Said Fayaz Sadat | Sprint 2 | Book Library#438
London | 26-ITP-Jan | Said Fayaz Sadat | Sprint 2 | Book Library#438fayaz551 wants to merge 4 commits intoCodeYourFuture:mainfrom
Conversation
- Corrected variable reference from 'library' to 'myLibrary'. - Fixed logic bug where title input was being used for the author field. - Fixed delete button functionality and event listener naming. - Corrected read status display logic to show "Yes" when checked.
- Changed input types for title and author to 'text' for standard validation. - Ensured script and bootstrap links are correctly ordered for DOM loading.
- Adjusted form-group dimensions for better visibility. - Added spacing and alignment for table action buttons.
This comment has been minimized.
This comment has been minimized.
fayaz551
changed the title
This comment has been minimized.
This comment has been minimized.
|
Your PR title is in the wrong format. That's why the validation bot could not recognise it. |
fayaz551
changed the title
This comment has been minimized.
This comment has been minimized.
fayaz551
changed the title
Changed the title to correct format, validation passed now. |
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
You have fixed the explicit bugs, but the code still have some rooms to improve.
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.
| myLibrary.push(book1); | ||
| myLibrary.push(book2); | ||
| render(); | ||
| // render() is called in the window load listener, so no need to call it here twice |
There was a problem hiding this comment.
You can delete any unnecessary code.
There was a problem hiding this comment.
Removed redundant render() call in populateStorage to follow DRY principles.
Refactored render() to clear table body efficiently using innerHTML
cjyuan
added
the
Reviewed
fayaz551
added
Needs Review
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?
| @@ -1 +1 @@ | |||
| let myLibrary = []; | |||
There was a problem hiding this comment.
Safer to declare myLibrary using const.
| const titleInput = document.getElementById("title"); | ||
| const authorInput = document.getElementById("author"); | ||
| const pagesInput = document.getElementById("pages"); | ||
| const checkInput = document.getElementById("check"); |
There was a problem hiding this comment.
Common practice is to declare all shared constants/variables at the beginning of the file (before function definition).
| return; | ||
| } | ||
|
|
||
| const book = new Book(title, author, pages, checkInput.checked); |
There was a problem hiding this comment.
-
Can
pagesbe a decimal number? -
What is the valid range of page count?
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
fixed bugs and implemented the fixes
Questions
n/a