Manchester | 25-ITP-May | Jennifer Isidienu | Sprint 2 | Book-Library #314
Conversation
JenniferIsidienu
added
Needs Review
There was a problem hiding this comment.
- Can you change the base branch of this PR from CYF's
book-libraryto CYF'smain?
- 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.
cjyuan
added
Reviewed
|
Thank you. I've changed to CYF main. I'll go through the general feedback to see where I can improve my code. |
JenniferIsidienu
added
the
Needs Review
debugging/book-library/script.js
Outdated
| ) { | ||
| 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); |
There was a problem hiding this comment.
-
Do you want to keep leading and trailing space characters in
titleandauthor? -
pages.valueis a string, and it can be any string value accepted by the<input type="number">element. -
Consider pre-process/sanitize/normalize input and store them in some variables first, and reference the variables in other parts of the function.
There was a problem hiding this comment.
Alright, I will make the corrections.
| delBut.addEventListener("clicks", function () { | ||
| alert(`You've deleted title: ${myLibrary[i].title}`); | ||
| // Delete button | ||
| let deleteCell = row.insertCell(4); |
There was a problem hiding this comment.
Can you think of the pros and cons of these two approaches for creating cells within a row?
- Keeping all the cell creation code in one location, like the original code does.
- Scattering the cell creation code across different locations, like what you did.
There was a problem hiding this comment.
Keeping it in one location will make it easier to read. I'll correct mine.
There was a problem hiding this comment.
By "keeping all cell creation code in one location", I meant
row.insertCell(0).innerText = book.title;
row.insertCell(1).innerText = book.author;
row.insertCell(2).innerText = book.pages;
// Could use const instead of let
const readCell = row.insertCell(3);
const deleteCell = row.insertCell(4);
...
This way, it is easy to find out there are 5 cells created.
cjyuan
removed
the
Needs Review
JenniferIsidienu
added
the
Needs Review
| delBut.addEventListener("clicks", function () { | ||
| alert(`You've deleted title: ${myLibrary[i].title}`); | ||
| // Delete button | ||
| let deleteCell = row.insertCell(4); |
There was a problem hiding this comment.
By "keeping all cell creation code in one location", I meant
row.insertCell(0).innerText = book.title;
row.insertCell(1).innerText = book.author;
row.insertCell(2).innerText = book.pages;
// Could use const instead of let
const readCell = row.insertCell(3);
const deleteCell = row.insertCell(4);
...
This way, it is easy to find out there are 5 cells created.
| } | ||
|
|
||
| let book = new Book(trimmedTitle, trimmedAuthor, pagesNumber, check.checked); |
There was a problem hiding this comment.
pageNumber could be a value like 3.1416.
|
Changes are good. Code could still be improved but I am sure you can easily fix them. |
cjyuan
added
Complete
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.