Skip to content

ITP London - Jan 2025 | Samunta Sunuwar| Module Data Flows | Book Library #203

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

ITP London - Jan 2025 | Samunta Sunuwar| Module Data Flows | Book Library #203

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
93dff77
Index Html
Samunta Apr 16, 2025
1470a9f
Script.Js
Samunta Apr 16, 2025
c1fd26d
Fixing errors according to https://validator.w3.org/. (In HTML5, the …
Samunta Apr 19, 2025
19208f1
Refactoring my logic for input validation, removed .value to be null …
Samunta Apr 19, 2025
18ade97
Refactoring my render function by adding innerHTML = "" to wipe all r…
Samunta Apr 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 7 additions & 10 deletions debugging/book-library/index.html
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job in fixing most of the errors in the HTML code.

According to https://validator.w3.org/, there are still some errors in the HTML code. Can you fix these errors?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback, I have made changes as requested.

Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
<!DOCTYPE html>
<html>
<head>
<title> </title>
<!-- Adding an appropriate title -->
<title>Sam's Book Library</title>
<meta
charset="utf-8"
name="viewport"
Expand Down Expand Up @@ -30,16 +31,18 @@ <h1>Library</h1>
<div id="demo" class="collapse">
<div class="form-group">
<label for="title">Title:</label>
<!-- fixing type from title to text -->
<input
type="title"
type="text"
class="form-control"
id="title"
name="title"
required
/>
<!-- fixing type from author to text -->
<label for="author">Author: </label>
<input
type="author"
type="text"
class="form-control"
id="author"
name="author"
Expand Down Expand Up @@ -81,13 +84,7 @@ <h1>Library</h1>
</tr>
</thead>
<tbody>
<tr>
<td></td>
<td></td>
<td></td>
<td></td>
<td></td>
</tr>
<!-- removing empty <tr> to prevent blank row on page load -->
</tbody>
</table>

Expand Down
46 changes: 29 additions & 17 deletions debugging/book-library/script.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Line 76:

    • The current method of assigning book titles to HTML elements can cause display issues if a title contains special character sequences like <i>.

  • Can you suggest a more consistent naming convention for the variables representing the two buttons, currently named changeBut and delButton?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback, I have made changes as requested.

Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,19 @@ function submit() {
if (
title.value == null ||
title.value == "" ||
//adding author validation
author.value == null ||
author.value == "" ||
pages.value == null ||
pages.value == ""
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of input validation,

  • Can .value be null? (Do we need to check someInputElement.value == null?)
  • Do you accept books to appear in the app as shown in this pic:
    image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback, I have made changes as requested.

alert("Please fill all fields!");
return false;
} else {
let book = new Book(title.value, title.value, pages.value, check.checked);
library.push(book);
//creating new book using the correct author and read status
let book = new Book(title.value, author.value, pages.value, check.checked);
//fixing typo(corrected variable name casing)
myLibrary.push(book);
render();
}
}
Expand All @@ -54,13 +59,15 @@ function render() {
let table = document.getElementById("display");
let rowsNumber = table.rows.length;
//delete old table
for (let n = rowsNumber - 1; n > 0; n-- {
//adding missing closing parenthesis
for (let n = rowsNumber - 1; n > 0; n--) {
table.deleteRow(n);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of deleting the table rows one by one, can you think of a more efficient way to remove all rows (except the <th>...</th>) in the table?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback, I have made changes as requested.

//insert updated row and cells
let length = myLibrary.length;
for (let i = 0; i < length; i++) {
let row = table.insertRow(1);
//appending the row to the end of the table
let row = table.insertRow(-1);
let titleCell = row.insertCell(0);
let authorCell = row.insertCell(1);
let pagesCell = row.insertCell(2);
Expand All @@ -74,30 +81,35 @@ function render() {
let changeBut = document.createElement("button");
changeBut.id = i;
changeBut.className = "btn btn-success";
wasReadCell.appendChild(changeBut);


//setting the button text based on read status (fixing logic by assigning correct boolean value)
let readStatus = "";
if (myLibrary[i].check == false) {
if (myLibrary[i].check == true) {
readStatus = "Yes";
} else {
readStatus = "No";
}
changeBut.innerText = readStatus;
wasReadCell.appendChild(changeBut);

changeBut.addEventListener("click", function () {
myLibrary[i].check = !myLibrary[i].check;
render();
});

//add delete button to every row and render again
let delButton = document.createElement("button");
delBut.id = i + 5;
deleteCell.appendChild(delBut);
delBut.className = "btn btn-warning";
delBut.innerHTML = "Delete";
delBut.addEventListener("clicks", function () {
alert(`You've deleted title: ${myLibrary[i].title}`);
myLibrary.splice(i, 1);
render();
});
// fixed: using one consistent button variable
let delButton = document.createElement("button");
delButton.id = `delete-${i}`; // optional: clearer ID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job in recognizing the need to ensure unique id value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

delButton.className = "btn btn-warning";
delButton.innerHTML = "Delete";
deleteCell.appendChild(delButton);

// using correct button variable name
delButton.addEventListener("click", function () {
alert(`You've deleted title: ${myLibrary[i].title}`);
myLibrary.splice(i, 1);
render();
});
}
}