London| ITP-May-2025| Zohreh Kazemianpour |Book library #252

zohrehKazemianpour · 2025-07-29T08:20:14Z

Learners, PR Template

Self checklist

I have committed my files one by one, on purpose, and for a reason
I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
I have tested my changes
My changes follow the style guide
My changes meet the requirements of this task

Changelist

This PR addresses all major functionality bugs in the Book Library:

✔ Fixed book display
Books now render correctly on page load
The default books appear as intended

✔ Corrected form submission
Properly captures all fields (title, author, pages, read status)
Fixed title/author mixup bug
Added validation for required fields

✔ Fixed delete functionality
Delete buttons now work reliably
Uses stable data-index system instead of fragile IDs

✔ Read status accuracy
Correctly saves and displays read/unread state
Toggle buttons update properly

Questions

Ask any questions you have for your reviewer.

… row positioning

- Add event listener for submit button instead of HTML onclick - Prevent default form submission behavior - Correct array name from 'library' to 'myLibrary' - Call render() after adding new books to update display - Fix validation to check for empty author field

- Reset title, author, and pages input fields to empty strings

…te buttons

debugging/book-library/index.html

debugging/book-library/script.js

cjyuan · 2025-08-03T18:40:03Z

debugging/book-library/script.js

-    let book = new Book(title.value, title.value, pages.value, check.checked);
-    library.push(book);
-    render();
+    let book = new Book(title.value, author.value, pages.value, check.checked);


Do you want to keep leading and trailing space characters in title, author, and pages?

The following page numbers are possible

No, I don't think it's a good idea to keep leading and trailing spaces. Thank you for catching this issue! I have now implemented better data validation. I used alert() for consistency here, but I am not sure if it is the best in terms of UX.

debugging/book-library/script.js

cjyuan · 2025-08-03T18:43:41Z

debugging/book-library/script.js

    table.deleteRow(n);
  }
  //insert updated row and cells
  let length = myLibrary.length;
  for (let i = 0; i < length; i++) {
-    let row = table.insertRow(1);
+    let row = table.insertRow(i+1);


We could also omit the argument in insertRow() if we wanted to append a row to the table.

debugging/book-library/script.js

cjyuan · 2025-08-03T18:46:41Z

debugging/book-library/script.js

+    readStatus = myLibrary[i].check ? "Yes" : "No";
    changeBut.innerText = readStatus;


The declaration of variable readStatus was deleted. You could also combined lines 82-83 into one statement without using readStatus.

cjyuan · 2025-08-03T18:48:21Z

debugging/book-library/script.js

      alert(`You've deleted title: ${myLibrary[i].title}`);
-      myLibrary.splice(i, 1);
+      const index = parseInt(this.dataset.index)
+      myLibrary.splice(index, 1);


The alert message is shown before the book is actually deleted; the deletion only occurs after the alert dialog is dismissed. This introduces a risk that the operation may not complete (e.g., if the user closes the browser before dismissing the alert).

In general, it’s better to display a confirmation message only after the associated operation has successfully completed.

…l tag - Fix meta tag structure (charset and viewport) - Change invalid input types (title/author → text) - Remove trailing slashes from void elements

…s logic

zohrehKazemianpour · 2025-08-04T14:43:44Z

Hi @cjyuan, thank you so much as always, for the valuable feedback. I have updated the code based on your comments. Could you please review when you have a moment? I appreciate your insights!

cjyuan

Changes look great! Well done!

cjyuan · 2025-08-04T16:10:22Z

debugging/book-library/script.js

  ) {
    alert("Please fill all fields!");
    return false;
-  } else {
-    let book = new Book(title.value, author.value, pages.value, check.checked);
+  } 
+  if (isNaN(parseInt(pages.value)) || parseInt(pages.value) <= 0) {
+    alert("Please enter a valid positive number for pages!");
+    return false;
+  }  
+    let book = new Book(title.value.trim(), author.value.trim(), parseInt(pages.value), check.checked);


For better performance (reduce number of function calls) and reducing the chance of using raw input accidently, we could stored the pre-processed/sanitized/normalized input in some variables first, and reference the variables in other part of the function.

const cleanTitle = title.value.trim(); ... const pageNum = parseInt(pages.value); ... const book = new Book(cleanTitle, cleanAuthor, pageNum, check.checked);

cjyuan · 2025-08-04T16:14:49Z

debugging/book-library/script.js

    myLibrary.push(book1);
    myLibrary.push(book2);
- }
+  }
 }

 const title = document.getElementById("title");


Using descriptive and consistent suffixes (like El, Input, Btn, Form, etc.) for variables that store DOM elements can improve code readability and maintainability.

zohrehKazemianpour · 2025-08-07T09:33:28Z

Changes look great! Well done!

Thank you so much for the complete label. I appreciate the time and energy you put into reviewing our coursework.

cjyuan and others added 10 commits May 6, 2025 14:44

Update pull_request_template.md (CodeYourFuture#223)

32cd65e

Fix the link to Style Guide in PR template

f71f87a

update index.html

11a9611

fix: use author name instead of title

a5245d8

Fix render function bugs: missing parenthesis, variable mismatch, and…

483a1e5

… row positioning

feat: clear form after successful book submission

d9a959e

- Reset title, author, and pages input fields to empty strings

fix :delBut eventListener to "click" instead of "clicks"

4b35ce9

refactor: replace button IDs with data-index attributes for Read/Dele…

ced4f5c

…te buttons

Fix incorrect logic for checkbox read status

3cd97df

zohrehKazemianpour added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 29, 2025

cjyuan reviewed Aug 3, 2025

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 3, 2025

zohrehKazemianpour added 8 commits August 4, 2025 11:22

fix(html): resolve W3C validation errors: - Add lang attribute to htm…

61a36b8

…l tag - Fix meta tag structure (charset and viewport) - Change invalid input types (title/author → text) - Remove trailing slashes from void elements

fix: convert page numbers to number data type

e5aebaf

feat: add validation for page numbers and trim whitespace

0ee09f5

refactor: optimize table clearing by using innerHTML instead of loop

ebd3fc1

Fixed book duplication issue in render() function"

6c1803e

Replace innerHTML with textContent for security

fd8786a

Refactor: Remove redundant readStatus variable and combine read-statu…

bb948ae

…s logic

refactor: replace alert() with confirm() before deletion

7203bd7

zohrehKazemianpour added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Aug 4, 2025

cjyuan reviewed Aug 4, 2025

View reviewed changes

cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 4, 2025

zohrehKazemianpour mentioned this pull request Aug 5, 2025

[TECH ED] Book Library zohrehKazemianpour/My-Coursework-Planner#131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

London| ITP-May-2025| Zohreh Kazemianpour |Book library #252

London| ITP-May-2025| Zohreh Kazemianpour |Book library #252

Uh oh!

zohrehKazemianpour commented Jul 29, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

cjyuan Aug 3, 2025

Uh oh!

zohrehKazemianpour Aug 4, 2025

Uh oh!

Uh oh!

cjyuan Aug 3, 2025

Uh oh!

Uh oh!

cjyuan Aug 3, 2025

Uh oh!

cjyuan Aug 3, 2025

Uh oh!

zohrehKazemianpour commented Aug 4, 2025

Uh oh!

cjyuan left a comment

Uh oh!

cjyuan Aug 4, 2025

Uh oh!

cjyuan Aug 4, 2025

Uh oh!

zohrehKazemianpour commented Aug 7, 2025

Uh oh!

Uh oh!

		readStatus = myLibrary[i].check ? "Yes" : "No";
		changeBut.innerText = readStatus;

Uh oh!

London| ITP-May-2025| Zohreh Kazemianpour |Book library #252

Are you sure you want to change the base?

London| ITP-May-2025| Zohreh Kazemianpour |Book library #252

Uh oh!

Conversation

zohrehKazemianpour commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

Changelist

Questions

Uh oh!

Uh oh!

Uh oh!

cjyuan Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

zohrehKazemianpour Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjyuan Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjyuan Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

cjyuan Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

zohrehKazemianpour commented Aug 4, 2025

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

cjyuan Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

zohrehKazemianpour commented Aug 7, 2025

Uh oh!

Uh oh!

zohrehKazemianpour commented Jul 29, 2025 •

edited

Loading