-
-
Notifications
You must be signed in to change notification settings - Fork 73
LONDON_JAN25 | KHALIL ALI | MODULE_DATA_FLOWS | SPRINT 2 | BOOK_LIBRARY #204
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
base: main
Are you sure you want to change the base?
Conversation
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.
According to https://validator.w3.org/, there are errors in your index.html
. Can you fix these errors?
I also suggested some improvements for you to consider.
debugging/book-library/script.js
Outdated
for (let n = rowsNumber - 1; n > 0; n-- { | ||
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.
-
In terms of input validation,
- Are all input properly checked?
- Can
.value
benull
? (Do we need to checksomeInputElement.value == null
?) - Do you want to allow book title and author name to contain only space characters?
- What if a user enters an invalid page number in the "pages" input field?
-
Lines 75, 93:
- Are the values assigned to these
id
attributes unique? - Is there any need to assign an id attribute to either buttons?
- Are the values assigned to these
-
Line 69:
- The current method of assigning book titles to HTML elements can cause display issues if a title contains special character sequences like
<i>
.
- The current method of assigning book titles to HTML elements can cause display issues if a title contains special character sequences like
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. I have checked all input properly
- ii.
.value
can not be null. - iii.
.trim()
deletes all spaces - iv. the "pages" input should not be empty and should be a positive integer
id attributes are not unique
There is no need to assign id attributes to either buttons
you are right. I used textContent
instead of innerhtml
I used https://validator.w3.org/ to check my HTML file. |
Learners, PR Template
Self checklist
Changelist
Fix all book library project bugs.
Questions
Ask any questions you have for your reviewer.