-
-
Notifications
You must be signed in to change notification settings - Fork 160
LONDON | MAY 2025 | JESUS DEL MORAL | DATA FLOWS | BOOK LIBRARY #302
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
LONDON | MAY 2025 | JESUS DEL MORAL | DATA FLOWS | BOOK LIBRARY #302
Conversation
cjyuan
left a comment
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.
Can you check if any of this general feedback can help you further improve your code?
https://github.com/cjyuan/Module-Data-Flows/blob/book-library-feedback/debugging/book-library/feedback.md
Doing so can help me speed up the review process. Thanks.
|
Hi @cjyuan Thank you |
cjyuan
left a comment
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?
debugging/book-library/script.js
Outdated
| if (!cleanTitle|| !cleanAuthor) { // title and author doesn't allowed to contain only space characters | ||
| alert("Title and Author cannot be empty or spaces only."); | ||
| return false; | ||
| } else if (isNaN(cleanPages) || cleanPages <= 0) { // the value of type of pages must be a integer | ||
| alert("Pages must be a positive whole number."); | ||
| return false; | ||
| } else { |
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.
When an if block contains a return statement, we could choose not to use else.
| } else { | ||
| let book = new Book(title.value, title.value, pages.value, check.checked); | ||
| library.push(book); | ||
| let book = new Book(cleanTitle, cleanAuthor, pages.value, check.checked); //// title and author be allowed to contain leading or trailing space characters, we storage using parseInt |
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.
pages.value could be "3e1" (which could pass the check on line 39).
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.
@cjyuan
I fixed all the errors
| table.deleteRow(n); | ||
| } | ||
| // Keep only the header row | ||
| table.innerHTML = table.rows[0].outerHTML; |
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.
Since the <table> also contains a <tbody> child element. The statement on line 60 will remove the <tbody> element.
An alternative approach is the clear <tbody> directly.
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.
No change?
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 removed from index.html
|
Your PR's title isn't in the expected format. Please check its title is in the correct format, and update it. Reason: Sprint part (DATA FLOWS) doesn't match expected format (example: 'Sprint 2', without quotes) |
cjyuan
left a comment
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.
Changes look good.
| alert("Title and Author cannot be empty or spaces only."); | ||
| return false; | ||
|
|
||
| } if (isNaN(cleanPages) || cleanPages <= 0) { // the value of type of pages must be a integer |
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.
Without else, common practice is to start an if block on a separate line.
| alert("Pages must be a positive whole number."); | ||
| return false; | ||
| } | ||
| let book = new Book(cleanTitle, cleanAuthor, pages.value, check.checked); //// title and author be allowed to contain leading or trailing space characters, we storage using parseInt |
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.
Why not use cleanPages? pages.value is a string and can still have leading and trailing space characters.
| table.deleteRow(n); | ||
| } | ||
| // Keep only the header row | ||
| table.innerHTML = table.rows[0].outerHTML; |
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.
No change?
|
Your PR's title isn't in the expected format. Please check its title is in the correct format, and update it. Reason: Sprint part (DATA FLOWS) doesn't match expected format (example: 'Sprint 2', without quotes) |
2 similar comments
|
Your PR's title isn't in the expected format. Please check its title is in the correct format, and update it. Reason: Sprint part (DATA FLOWS) doesn't match expected format (example: 'Sprint 2', without quotes) |
|
Your PR's title isn't in the expected format. Please check its title is in the correct format, and update it. Reason: Sprint part (DATA FLOWS) doesn't match expected format (example: 'Sprint 2', without quotes) |
topics: debugging, DOM
My website should be able to:
View a list of books that I've read
Add books to a list of books that I've read
Including title, author, number of pages and if I've read it
If any of the information is missing it shouldn't add the book and should show an alert
Remove books from my list
Bugs to be fixed
Website loads but doesn't show any books
Error in console when you try to add a book
It uses the title name as the author name
Delete button is broken
When I add a book that I say I've read - it saves the wrong answer
I think there are other some other small bugs in my code...but I'm lazy so I can't fix them all.
I wish somebody would help me!