-
-
Notifications
You must be signed in to change notification settings - Fork 161
London | 25-ITP-May | Houssam Lahlah | Sprint 2 | feature/book library #313
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,8 +37,10 @@ function submit() { | |
| alert("Please fill all fields!"); | ||
| return false; | ||
| } else { | ||
| let book = new Book(title.value, title.value, pages.value, check.checked); | ||
| library.push(book); | ||
| //fix: change title.value to author.value | ||
| let book = new Book(title.value, author.value, pages.value, check.checked); | ||
| //Fix: change library.push(book) to myLibrary.push(book). | ||
| myLibrary.push(book); | ||
| render(); | ||
| } | ||
| } | ||
|
|
@@ -54,7 +56,9 @@ function render() { | |
| let table = document.getElementById("display"); | ||
| let rowsNumber = table.rows.length; | ||
| //delete old table | ||
| for (let n = rowsNumber - 1; n > 0; n-- { | ||
| // Fix : add a ) to the end of the for loop condition | ||
| // to avoid syntax error. | ||
| for (let n = rowsNumber - 1; n > 0; n--) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch of the syntax error here as well. |
||
| table.deleteRow(n); | ||
| } | ||
| //insert updated row and cells | ||
|
|
@@ -76,7 +80,9 @@ function render() { | |
| changeBut.className = "btn btn-success"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whilst this isn’t a bad variable name, it is good practice to have clear variable names so the purpose can be inferred at a quick glance. For example, |
||
| wasReadCell.appendChild(changeBut); | ||
| let readStatus = ""; | ||
| if (myLibrary[i].check == false) { | ||
| // Fix: change myLibrary[i].check to myLibrary[i].check == true | ||
| // to check if the book is read or not. | ||
| if (myLibrary[i].check == true) { | ||
| readStatus = "Yes"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you build applications with status updates, it is good practice to store these statuses in variables/objects. For a small application like this, it is not necessary but as your codebase grows bigger, it becomes easier to slot in the status variable than constantly type in a string (typos could happen). This is something to note. |
||
| } else { | ||
| readStatus = "No"; | ||
|
|
@@ -89,12 +95,15 @@ function render() { | |
| }); | ||
|
|
||
| //add delete button to every row and render again | ||
| // Fix : | ||
| // change delBut to delButton because we declare it. | ||
| // The event listener is "clicks" instead of "click". | ||
| let delButton = document.createElement("button"); | ||
| delBut.id = i + 5; | ||
| deleteCell.appendChild(delBut); | ||
| delBut.className = "btn btn-warning"; | ||
| delBut.innerHTML = "Delete"; | ||
| delBut.addEventListener("clicks", function () { | ||
| delButton.id = i + 5; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See. You used a better variable name here, so try to keep it consistent. |
||
| deleteCell.appendChild(delButton); | ||
| delButton.className = "btn btn-warning"; | ||
| delButton.innerHTML = "Delete"; | ||
| delButton.addEventListener("click", function () { | ||
| alert(`You've deleted title: ${myLibrary[i].title}`); | ||
| myLibrary.splice(i, 1); | ||
| render(); | ||
|
|
||
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.
This is nicely done.