-
-
Notifications
You must be signed in to change notification settings - Fork 143
ITP_LONDON | PRISCILLA_EMEBO | Module-Data-Flows | Book library #183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ITP_LONDON | PRISCILLA_EMEBO | Module-Data-Flows | Book library #183
Conversation
cjyuan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some improvements you can still make to the JS code.
Also, when I checked the code in index.html at https://validator.w3.org/, it indicated some errors.
debugging/book-library/script.js
Outdated
| author.value == null || | ||
| author.value == "" || | ||
| pages.value == null || | ||
| pages.value == "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of input validation,
- Are all input properly checked?
- Can
.valuebenull? (Do we need to checksomeInputElement.value == null?) - What if a user enters only space characters in the "title" input field?
- What if a users enters
-1or3.1416in the "pages" input field?
debugging/book-library/script.js
Outdated
| for (let n = rowsNumber - 1; n > 0; n-- ){ | ||
| table.deleteRow(n); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of deleting the table rows one by one, can you think of a more efficient way to remove all rows (except the <th>...</th>) in the table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 71: With the way the book's title is assigned to an HTML element, a book with a title containing special character sequence such as <i> can possibly ruin the display.
Lines 77, 95:
- Is the value assigned to these
idattribute unique? - Is there a need to assign an id attribute to
delButton? - Is there a need to assign an id attribute to
changeBut(at line 72)?
Lines 76, 94:
- Can you think of a more consistent way to name the variables representing the two buttons,
changeButanddelButton?

Learners, PR Template
Self checklist
Changelist
This PR implements a simple Book Library web app using HTML, CSS, and JavaScript. Users can add books with a title, author, page count, and read status. Each book is displayed in a table, with options to toggle its read status or delete it
Questions
Ask any questions you have for your reviewer.