Sheffield| 26-ITP-Jan| Mona- Eltantawy | Sprint 2 | Book library#472
Sheffield| 26-ITP-Jan| Mona- Eltantawy | Sprint 2 | Book library#472Mona-Eltantawy wants to merge 9 commits intoCodeYourFuture:mainfrom
Conversation
Mona-Eltantawy
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Flows/blob/general-review-feedback/debugging/book-library/feedback.md
Doing so can help me speed up the review process. Thanks.
cjyuan
added
Reviewed
- Remove unnecessary empty placeholder row from table - Add consistent 'El' suffix to all DOM node variable names for clarity - Improve input validation with explicit NaN check and page count range validation (1-9999) - Update error message to reflect validation constraints
…alerts, add non-blocking notifications
…ity, and validate with W3C
I reviewed the code according the feedback. |
Mona-Eltantawy
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Code looks good. I just have a few questions.
| const authorCell = document.createElement("td"); | ||
| const pagesCell = document.createElement("td"); | ||
| const readCell = document.createElement("td"); | ||
| const deleteCell = document.createElement("td"); |
There was a problem hiding this comment.
Why use this approach instead of the original approach to create table cell?
const titleCell = row.insertCell(0);
const authorCell = row.insertCell(1);
const pagesCell = row.insertCell(2);
const wasReadCell = row.insertCell(3);
const deleteCell = row.insertCell(4);There was a problem hiding this comment.
actually I used the original one and fixed it in my first edit to the code and when I passed it to an AI tool for review it fixed it to this one and I considered it when no issues came out in validation. I understand that there are differences between the two approaches but I considered the validation impact.
There was a problem hiding this comment.
In you opinion, which approach is better, and why?
There was a problem hiding this comment.
for me the document .create element the original one and this because it's the one I'm confident to use right now actually but I'm afraid to change it to affect the validation again
| render(); | ||
| // ✅ Handle form submit (NO inline onclick) | ||
| formEl.addEventListener("submit", (e) => { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
What is the purpose of this statement (line 31)?
There was a problem hiding this comment.
this line helped sorting the issue of missing the data submitted to the form with every page reload that was one of the issues I struggled with
There was a problem hiding this comment.
That's not what e.preventDefault() does. Can you look it up what it does?
| <label for="pages">Pages:</label> | ||
| <input type="number" class="form-control" id="pages" required min="1" max="9999"> | ||
|
|
||
| <div class="form-check mt-2 mb-3"> |
There was a problem hiding this comment.
What does class mt-2 and mb-3 do?
There was a problem hiding this comment.
they are mainly styling and doesn't affect the javascript but it makes the style for the checkbox looks clear. as the mt-2 add space above the dive and prevents it from sticking too close to the page input.
Also mt-3 add space blow that section to separate it from the submit button.
cjyuan
removed
the
Needs Review
Mona-Eltantawy
added
the
Needs Review
cjyuan
removed
the
Needs Review
Mona-Eltantawy
added
the
Needs Review
|
Code looks good. I will mark the PR complete first. Please respond to the remaining two comments when you have time. |
cjyuan
added
Complete
|
Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it. |
3 participants
No description provided.