West Midlands | 25 Sep ITP | Iswat Bello | Sprint 2 | Book library #349

Iswanna · 2025-12-09T13:37:45Z

Learners, PR Template

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Changelist

I fixed the initial render so books show on load (closing the for-loop parenthesis), resolved the add-book console error by pushing to myLibrary, corrected the add-book form to use author.value instead of the title for the author, repaired the Delete button by using a consistent variable name and the proper click event, and corrected the read-status logic so that clicking Read shows Yes in the Read column, and leaving it unchecked shows No.

Questions

Hi. Please could you review my PR? I’d really appreciate your feedback.

…ndering

…e delete action

… array

… 40)

…and unchecked shows “No”

…double render on load

cjyuan

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.

Iswanna · 2025-12-10T02:56:50Z

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.

Thank you @cjyuan for sharing the link. I'll go through each point carefully and work on the improvements suggested in the feedback file.

…anage all rows

…nput type="author"/> to type="text"

…add proper addEventListener on submit button to avoid global scope usage and ensure modern JS practices

…listener in script.js

… page count is stored consistently as a number, and remove unused id attributes from dynamically created buttons

…ames (titleInput, authorInput, pagesInput, readCheckbox) to avoid confusion with Book object properties

- Replace per-row deletion with a single tbody.innerHTML = "" operation - Preserves the thead header and reduces DOM work during updates - No functional UI changes; improves performance on larger lists

- Replace pre-delete alert with confirm dialog - Delete immediately upon confirmation and re-render - Display temporary success message that auto-fades without blocking UI

…” in populateStorage()

…on click

…e Submit a button to pass W3C validation

Iswanna · 2025-12-10T15:13:34Z

Hi @cjyuan.

I have made changes to my codebase based on the feedback in the link you shared. Could you please review the updated code?

Thank you!

cjyuan

Changes look great.

Input validation could use some improvement.

cjyuan · 2025-12-10T15:50:46Z

debugging/book-library/script.js

+function showSuccess(message) {
+  const alertEl = document.createElement("div");
+  alertEl.className = "alert alert-success";
+  alertEl.textContent = message;
+  alertEl.style.position = "fixed";
+  alertEl.style.top = "1rem";
+  alertEl.style.right = "1rem";
+  alertEl.style.zIndex = "1050";
+  document.body.appendChild(alertEl);
+
+  setTimeout(() => {
+    alertEl.style.transition = "opacity 0.3s";
+    alertEl.style.opacity = "0";
+    setTimeout(() => alertEl.remove(), 300);
+  }, 2500);
+}


(Optional)
Could consider designating an element in index.html to display the message. This way, we can have more control where to position the element.

Separation of concern -- We should avoid specifying CSS styles in JS or in HTML file. (Assigning CSS class is ok)

It is possible to create the effect of "making the foreground visible for a fixed amount of time" using only CSS. And then in JS we can just add/remove CSS class to trigger the animation without using setTimeout().

Thank you for the suggestion. I’ve made the updates to the code based on your feedback.

cjyuan · 2025-12-10T15:52:07Z

debugging/book-library/script.js

+  }
+
+  // Validate pages
+  if (Number.isNaN(pages) || pages <= 0) {


Some invalid "number of pages" can still pass this check.

Thank you for the feedback. I have updated the code to fix this bug.

cjyuan · 2025-12-10T15:56:07Z

You should also change the label to "Needs review" when the PR is ready to be re-reviewed.

…ove inline styles/timeouts - Add dedicated notice container (#alerts) for controlled placement - Move toast styling to CSS (.notice-root, .notice, .notice--auto-hide, keyframes) - Update showSuccess to add classes and remove on animationend (no setTimeout) - Keep existing behaviors (submit prevention, delete confirm, table re-render) intact

- Use valueAsNumber and input validity (valueMissing, rangeUnderflow, stepMismatch) - Reject non-numeric and decimal values; enforce min=1 and step=1 - Preserve existing submit handling and notice behavior

Iswanna · 2025-12-11T10:49:45Z

You should also change the label to "Needs review" when the PR is ready to be re-reviewed.

Thanks for highlighting that! I’ll make sure to set the “Needs review” label when the PR is ready again.

Iswanna · 2025-12-11T10:53:26Z

Hi @cjyuan.

Thanks for the feedback! I’ve updated the codebase and would appreciate another review.

cjyuan

Changes look good.

cjyuan · 2025-12-11T11:54:02Z

debugging/book-library/script.js

+  const pages = pagesInput.valueAsNumber;
+  const v = pagesInput.validity;
+
+  if (v.valueMissing || !Number.isFinite(pages)) {
+    alert("Pages is required and must be a number.");
+    return;
+  }
+  if (v.rangeUnderflow || pages < 1) {
+    alert("Pages must be at least 1.");
+    return;
+  }
+  // Enforce whole numbers only (reject decimals like 3.5)
+  if (v.stepMismatch || !Number.isInteger(pages)) {
+    alert("Pages must be a whole number.");
    return;
  }


Just checking !Number.isInteger(pages) || pages < 1 would be enough.

Iswanna · 2025-12-11T12:11:19Z

Thank you so much @cjyuan, for reviewing my PR and giving detailed feedback. I really appreciate it.

Iswanna added 🏕 Priority Mandatory This work is expected 📅 Sprint 2 Assigned during Sprint 2 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Data-Flows The name of the module. labels Dec 9, 2025

Iswanna added 6 commits December 9, 2025 14:19

Fix syntax error in for-loop (line 57) that blocked book list from re…

e2bf444

…ndering

Fix: rename delButton to delBut and correct event to 'click' to enabl…

c39e375

…e delete action

Rename library to myLibrary in submit() so new books are added to the…

c794174

… array

fix: use author.value for author field when creating a new Book (line…

436136b

… 40)

fix(render): correct read status display so checked=true shows “Yes” …

dccb8a5

…and unchecked shows “No”

Remove redundant render() call in populateStorage (line 19) to avoid …

07c6620

…double render on load

Iswanna force-pushed the feature/book-library branch from 9f10ac7 to 07c6620 Compare December 9, 2025 14:27

cjyuan reviewed Dec 9, 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 Dec 9, 2025

Iswanna added 13 commits December 10, 2025 11:47

Remove placeholder table row <tr> from <tbody> to allow render() to m…

085a373

…anage all rows

Fix invalid HTML input types by changing <input type="title"/> and <i…

4bf7b31

…nput type="author"/> to type="text"

Switch script.js to type="module", remove inline onclick="submit()", …

d9870cf

…add proper addEventListener on submit button to avoid global scope usage and ensure modern JS practices

Add id="submitBtn" to the submit button to enable attaching an event …

3acd315

…listener in script.js

Refactor code to remove redundant null checks on input values, ensure…

dae6c85

… page count is stored consistently as a number, and remove unused id attributes from dynamically created buttons

Refactor variable names for clarity: DOM inputs now use descriptive n…

a6619ba

…ames (titleInput, authorInput, pagesInput, readCheckbox) to avoid confusion with Book object properties

fix(js): add full input validation for author and page count

d413566

refactor(js): optimize table clearing using tbody.innerHTML

c784153

Clear rows via tbody.innerHTML to preserve header and speed up re-render

4757d2f

- Replace per-row deletion with a single tbody.innerHTML = "" operation - Preserves the thead header and reduces DOM work during updates - No functional UI changes; improves performance on larger lists

ux(delete): confirm before deletion and show non-blocking success alert

0a4c4ae

- Replace pre-delete alert with confirm dialog - Delete immediately upon confirmation and re-render - Display temporary success message that auto-fades without blocking UI

Correct book title spelling from “Robison Crusoe” to “Robinson Crusoe…

757fcd3

…” in populateStorage()

Fix(form): prevent default submit to avoid page reload on Submit butt…

2b98534

…on click

Chore(html): add lang, split meta tags, set meaningful title, and mak…

63652a8

…e Submit a button to pass W3C validation

Iswanna requested a review from cjyuan December 10, 2025 15:13

cjyuan reviewed Dec 10, 2025

View reviewed changes

Iswanna added 4 commits December 11, 2025 10:01

Add #alerts container and move the dependency scripts to end of the body

7c28cc2

Fix indentation inconsistency in index.html

9526485

fix(validation): require pages to be a positive integer

2417ca4

- Use valueAsNumber and input validity (valueMissing, rangeUnderflow, stepMismatch) - Reject non-numeric and decimal values; enforce min=1 and step=1 - Preserve existing submit handling and notice behavior

Iswanna 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 Dec 11, 2025

Iswanna requested a review from cjyuan December 11, 2025 10:54

cjyuan reviewed Dec 11, 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 Dec 11, 2025

Iswanna mentioned this pull request Dec 15, 2025

West Midlands | Data Flows | Iswat Bello Iswanna/My-Coursework-Planner#153

Open

Uh oh!

West Midlands | 25 Sep ITP | Iswat Bello | Sprint 2 | Book library #349

Are you sure you want to change the base?

West Midlands | 25 Sep ITP | Iswat Bello | Sprint 2 | Book library #349

Uh oh!

Conversation

Iswanna commented Dec 9, 2025

Learners, PR Template

Changelist

Questions

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Iswanna commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Iswanna commented Dec 10, 2025

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Iswanna Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjyuan Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Iswanna Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

cjyuan commented Dec 10, 2025

Uh oh!

Iswanna commented Dec 11, 2025

Uh oh!

Iswanna commented Dec 11, 2025

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Iswanna commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Iswanna commented Dec 10, 2025 •

edited

Loading

Iswanna Dec 11, 2025 •

edited

Loading

Iswanna commented Dec 11, 2025 •

edited

Loading