ITP London - Jan 2025 | Samunta Sunuwar| Module Data Flows | Book Library #203
Conversation
Samunta
commented
Apr 16, 2025
Samunta
commented
- I have committed my files one by one, on purpose, and for a reason
- I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
- I have tested my changes
- My changes follow the style guide
- My changes meet the requirements of this task
cjyuan
added
Review in progress
debugging/book-library/index.html
Outdated
There was a problem hiding this comment.
Good job in fixing most of the errors in the HTML code.
According to https://validator.w3.org/, there are still some errors in the HTML code. Can you fix these errors?
There was a problem hiding this comment.
Thank you for your feedback, I have made changes as requested.
debugging/book-library/script.js
Outdated
| title.value == null || | ||
| title.value == "" || | ||
| //adding author validation | ||
| author.value == null || | ||
| author.value == "" || | ||
| pages.value == null || | ||
| pages.value == "" | ||
| ) { |
There was a problem hiding this comment.
In terms of input validation,
- Can
.valuebenull? (Do we need to checksomeInputElement.value == null?) - Do you accept books to appear in the app as shown in this pic:
There was a problem hiding this comment.
Thank you for your feedback, I have made changes as requested.
debugging/book-library/script.js
Outdated
| for (let n = rowsNumber - 1; n > 0; n--) { | ||
| table.deleteRow(n); | ||
| } |
There was a problem hiding this comment.
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.
Thank you for your feedback, I have made changes as requested.
debugging/book-library/script.js
Outdated
| }); | ||
| // fixed: using one consistent button variable | ||
| let delButton = document.createElement("button"); | ||
| delButton.id = `delete-${i}`; // optional: clearer ID |
There was a problem hiding this comment.
Great job in recognizing the need to ensure unique id value.
There was a problem hiding this comment.
-
Line 76:
- 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
-
Can you suggest a more consistent naming convention for the variables representing the two buttons, currently named
changeButanddelButton?
There was a problem hiding this comment.
Thank you for your feedback, I have made changes as requested.
cjyuan
added
Reviewed
…<input> element is a void element, which means it should not have a closing tag (like </input>)
…and used .trim to ensure no white spaces are being added. Also added validation to reject input with negative or decimal numbers
…ows instantly, instead of triggering multiple reflows with deleteRow.
Samunta
added
Complete
