West Midlands | Samira Hekmati | Module-Data-Flows | Sprint-2 | Book-library

[samirahekmati]

Learners, PR Template

Self checklist

• 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

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

[cjyuan]

In terms of input validation,

1. Are all input properly checked?
2. Can .value be null? (Do we need to check someInputElement.value == null?)
3. What if a user enters only space characters in the "title" input field?
4. What if a users enters -1 or 3.1416 in the "pages" input field?

There are also some errors in the index.html.

[cjyuan]

• Do you think the value assigned to this id attribute is unique?
• Do you think there is a need to assign any id attribute to delButton?
• Do you think there is a need to assign any id attribute to changeBut (at line 79)?
• Can you think of a more consistent way to name the variables representing the two buttons?

[samirahekmati]

you're right @cjyuan The value i + 5 for the id attribute isn't guaranteed to be unique.
Instead of using i + 5 (which is not reliable),I can simply use the i value as the id for the delete button since it's guaranteed to be unique within the context of the current render.

[samirahekmati]

infact no need to assign an ID here. I am already using the i index inside the click event listener to know which book was clicked (since i uniquely identifies each book in the loop).
I can simply remove the assignment of the id for the delete button.

[samirahekmati]

Similarly to the delButton, there is no need to assign an id attribute to the changeBut button (line 79).

[samirahekmati]

changeBut → toggleReadButton: The button's purpose is to toggle the "read" status of the book, so a more descriptive name would help clarify its role.
delButton → deleteButton: This name is already clear but can be made consistent with the other button name by making it follow the same pattern.

[cjyuan]

Can you think of a more efficient way to remove all rows (except the <th>...</th>) in the table?

[samirahekmati]

Yes! simply clear all the rows except the header row by removing all child elements from the (or equivalent body section of the table). This avoids looping through the rows and directly empties the content of the table

[samirahekmati]

Can .value be null? (Do we need to check someInputElement.value == null?)
Answer: No, .value will not be null on a valid input element. The .value of an input field is an empty string ("") if the user has not entered anything, not null. So, checking for value == null is unnecessary.