London | 26-ITP-Jan | Boualem Larbi Djebbour | Sprint 2 | Book Library#482
London | 26-ITP-Jan | Boualem Larbi Djebbour | Sprint 2 | Book Library#482djebsoft wants to merge 1 commit intoCodeYourFuture:mainfrom
Conversation
djebsoft
added
the
Needs Review
Luro91
added
the
Review in progress
Luro91
left a comment
Luro91
left a comment
There was a problem hiding this comment.
Good job on fixing the Major bugs.
There are still some improvements that can be done
According to https://validator.w3.org/, there are errors in your index.html. Can you fix these errors?
| } | ||
| changeBut.innerText = readStatus; | ||
|
|
||
| changeBut.innerText = myLibrary[i].check ? "Yes" : "No"; |
| if ( | ||
| title.value == null || | ||
| title.value == "" || | ||
| author.value == null || | ||
| author.value == "" || | ||
| pages.value == null || | ||
| pages.value == "" | ||
| ) { | ||
| alert("Please fill all fields!"); | ||
| return false; | ||
| } else { | ||
| let book = new Book(title.value, title.value, pages.value, check.checked); | ||
| library.push(book); | ||
| let book = new Book(title.value, author.value, pages.value, check.checked); | ||
| myLibrary.push(book); | ||
| render(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Before using input in an app, we should properly preprocess it (even if basic validation has already been enforced in HTML):
- Input Validation - Check whether the input meets expected formats, types, and constraints
- Input Sanitization - Clean input to exclude unwanted or potentially harmful data
- Input Normalization/Conversion - Transform input into the correct data type or a standardized, usable format
Questions to consider:
- Should
titleandauthorbe allowed to contain only space characters? - Should
titleandauthorbe allowed to contain leading or trailing space characters? - What type of value should we use to store the page count?
- What kinds of input values (if any) should be rejected?
| for (let n = rowsNumber - 1; n > 0; n--) { | ||
| table.deleteRow(n); | ||
| } |
There was a problem hiding this comment.
Efficient approach to remove rows in <table>
In render(), instead of deleting table rows one by one, we could delete all rows in one operation by clearing their container.
|
|
||
| //add delete button to every row and render again | ||
| let delButton = document.createElement("button"); | ||
| let delBut = document.createElement("button"); |
There was a problem hiding this comment.
- Are the values assigned to the
idattributes of the delete button unique? - Is there any need to assign an
idattribute to the buttons?
| @@ -54,7 +55,7 @@ function render() { | |||
| let table = document.getElementById("display"); | |||
There was a problem hiding this comment.
When setting the text content of an HTML element, there are subtle but important differences between using .innerHTML, innerText, and textContent.
In render(), are the text values assigned to the appropriate property of each DOM node?
| alert(`You've deleted title: ${myLibrary[i].title}`); | ||
| myLibrary.splice(i, 1); |
There was a problem hiding this comment.
In the callback function of the delete button, the alert message is shown before the book is actually deleted;
the deletion only occurs after the alert dialog is dismissed. This introduces a risk that the operation may
not complete (e.g., if the user closes the browser before dismissing the alert).
In general, it's better to display a confirmation message only after the associated operation has successfully
completed.
Luro91
added
Reviewed
|
Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it. |
3 participants
Learners, PR Template
Self checklist
Changelist
fixed the bugs
Questions