London | Emmanuel Gessessew | Module-Data-Flows| WEEK 2 Book library project #124
Conversation
Emmanuelgessessew
added
the
Needs Review
OracPrime
added
the
Review in progress
|
It's always a good habit to try to use your code as an end user just before you submit a PR. In particular, there's the "does it work" test. In particular have you tried submitting books with different values of "Checked" for whether it was read or not. Or indeed does it look right for your initial two books? Can you see the problem here? |
|
I also when possible try to look at the diff of each changed file I submit, as this sometimes makes bugs more obvious than looking at the entire code. For example my previous comment might be most easily addressed by looking at 7239a23 and seeing if anything leaps off the page at you! |
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "liveServer.settings.port": 5502 | |||
There was a problem hiding this comment.
There is debate as to whether you should commit your .vscode file to version control. Generally it is things related to your own personal setup, and you don't want to force other users to share them. Google .gitignore to discover how to exclude files or folders from version control.
Sometimes you do want local configuration to be shared, but only do it intentionally!
|
|
||
| titleCell.innerHTML = myLibrary[i].title; | ||
| authorCell.innerHTML = myLibrary[i].author; | ||
| pagesCell.innerHTML = myLibrary[i].pages; |
There was a problem hiding this comment.
I'm pretty sure your code wasn't working when you made this commit, because there are a whole bunch of missing } at the end (which show up in the next commit). It's generally considered prudent to have committed code in a reasonable state of "cleanness" as others may check it out and try to use it. Less of an issue on a branch that only you are using, but something to bear in mind
| // Insert updated rows and cells | ||
| let length = myLibrary.length; | ||
| for (let i = 0; i < length; i++) { | ||
| let row = table.insertRow(1); |
There was a problem hiding this comment.
What does the 1 argument do here? How does the result differ if you leave it out? Which do you prefer?
| @@ -7,7 +7,7 @@ window.addEventListener("load", function (e) { | |||
|
|
|||
| function populateStorage() { | |||
| if (myLibrary.length == 0) { | |||
| let book1 = new Book("Robison Crusoe", "Daniel Defoe", "252", true); | |||
| let book1 = new Book("Robinson Crusoe", "Daniel Defoe", "252", true); | |||
There was a problem hiding this comment.
This is a good correction. Having accurate test data in software helps users and testers take it more seriously. So well done for bothering to correct this.
| }); | ||
| // Add a delete button to every row and render again | ||
| let delButton = document.createElement("button"); | ||
| delButton.id = i + 5; |
There was a problem hiding this comment.
What is this delButton.id = i+5 doing?
| }); | ||
| } | ||
| delButton.addEventListener("click", function () { | ||
| alert(`You've deleted title: ${myLibrary[i].title}`); |
There was a problem hiding this comment.
Explain to me where the value for i comes from here?
| alert(`You've deleted title: ${myLibrary[i].title}`); | ||
| myLibrary.splice(i, 1); | ||
| render(); | ||
| }); |
There was a problem hiding this comment.
If you can set a formatter such as prettier up on your vscode, you'll be able to easily format the source file so that the structure is clearer.
OracPrime
added
Reviewed
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Correcting input validation before adding books.
Using myLibrary instead of library.
Fixing title/author mismatch.
Ensuring the delete button works.
Correctly saving the "read" status.
Questions
Ask any questions you have for your reviewer.