London | 26-ITP-May | Zadri Abdule | Sprint 2 | Book - library by Zadri415 · Pull Request #520 · CodeYourFuture/Module-Data-Flows

Zadri415 · 2026-08-06T17:39:40Z

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

index.html — fix form input types and wiring (changed submit button to call addBook()).
script.js — fixes for seeding, add-book handling, read state (use boolean), rendering, toggle read, and delete logic.

Questions

N/A

cjyuan

Can you check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Flows/blob/general-review-feedback/debugging/book-library/feedback.md

Doing so can help me speed up the review process. Thanks.

cjyuan

Changes look good.

There are a few improvements you could still make.

cjyuan · 2026-08-13T20:59:00Z

+      #toast {
+        position: fixed;


Why not prepare the CSS in style.css?

cjyuan · 2026-08-13T21:02:58Z

-        />
+        <div class="col">
+          <label for="pages">Pages:</label>
+          <input type="number" class="form-control" id="pages" name="pages" min="1" required />


Could you find out why the browser is not enforcing the constraint min="1" required? Currently a user could submit page count suchas-123and12.345`.

cjyuan · 2026-08-13T21:06:03Z

+  // Preprocessing / validation:
+  // - reject empty or whitespace-only title/author (checked AFTER trim, not before)
+  // - reject non-numeric or non-positive page counts
+  if (!title || !author || !pagesRaw || !Number.isFinite(pages) || pages <= 0) {


Do you know why checking !pageRaw is optional?

What type of number should a page count be?

cjyuan · 2026-08-13T21:06:49Z

+  titleInput.value = "";
+  authorInput.value = "";
+  pagesInput.value = "";
+  checkInput.checked = false;


If the input elements are inside a <form>, we could also just reset the form with one function call.

cjyuan · 2026-08-13T21:08:48Z

+  const table = document.getElementById("display");
+  const tbody = table.getElementsByTagName("tbody")[0];


Note: We could also use document.querySelector() to select the specific tbody element directly.

cjyuan · 2026-08-13T21:11:17Z

+      const idx = myLibrary.indexOf(book);
+      if (idx === -1) return;


We could also obtain the index of the current book from the second parameter of the function pass to forEach() on line 66.

Thank you for the feedback. I have updated both files.

cjyuan · 2026-08-17T20:10:02Z

May I know why you closed this PR?

Zadri415 · 2026-08-17T20:18:35Z

Had issues with the branch not pushing some of recent commits. here's the other PR: #564

…

On Mon, Aug 17, 2026 at 9:10 PM CJ Yuan ***@***.***> wrote: *cjyuan* left a comment (CodeYourFuture/Module-Data-Flows#520) <#520 (comment)> May I know why you closed this PR? — Reply to this email directly, view it on GitHub <#520?email_source=notifications&email_token=BOUYTCQBPTFWEDZVLUCFDDL5KNRDDA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZRHE3DSMRRGQ42M4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5319692149>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BOUYTCT2JUCUW2UBOG4PC335KNRDDAVCNFSNUABFKJSXA33TNF2G64TZHM3TSOJSGQ2TQMRXHNEXG43VMU5TKMBYGM2TSOBYGQ22C5QC> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/BOUYTCWRSTX6RTQXGOPHBCT5KNRDDA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZRHE3DSMRRGQ42M4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android <https://github.com/notifications/mobile/android/BOUYTCQNWGUVZZTTGAKS4LL5KNRDDA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZRHE3DSMRRGQ42M4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today! You are receiving this because you modified the open/close state.Message ID: ***@***.***>

cjyuan · 2026-08-17T23:19:14Z

The validation boot does not recognise duplicated PRs for any backlog in ITP.

Currently you have two book library PRs. If you want to continue to work on this PR, you would need to close the other PR first. Otherwise the validation bot will reject this PR.

cjyuan

Changes look good.

There are some improvements you could still make though.

cjyuan · 2026-08-18T22:06:47Z

+  // Native "required"/"min"/"step" already ran before this handler fires
+  // (see bookForm's submit listener below), so this is a second layer
+  // for things HTML attributes can't express, like whitespace-only text.
+  if (!title || !author || !pagesRaw || !Number.isFinite(pages) || pages <= 0) {


Do you know why checking !pageRaw is optional?

Why not just check if pages is a whole number?

cjyuan · 2026-08-18T22:11:03Z

+bookForm.addEventListener("submit", (event) => {
+  event.preventDefault();
+  addBook();
+});
+
+document.addEventListener("DOMContentLoaded", () => {
+  populateStorage();
+  render();
+});


Good job in placing all code that runs once on page load at the end of the file. Doing so could make locating and managing them easier.

A better practice would be to keep them in a single function. For example, you could put it inside the page load callback or create a function named init() or setup() and call it once when the page loads.

correct form input types and submit wiring

e978b0b

Zadri415 added 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 Aug 6, 2026

cjyuan reviewed Aug 7, 2026

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 7, 2026

Zadri415 added 2 commits August 13, 2026 20:30

Update script.js

3114094

Update index.html

e597733

Zadri415 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 13, 2026

cjyuan reviewed Aug 13, 2026

View reviewed changes

cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 13, 2026

Zadri415 added 4 commits August 16, 2026 11:56

Update script.js

66eeeac

Update index.html

8a41b5d

Update index.html

7f5941f

Update script.js

ff58ce9

Zadri415 closed this Aug 17, 2026

cjyuan mentioned this pull request Aug 17, 2026

London | 26-ITP-May | Zadri Abdule | Sprint 2 | Book - library #564

Closed

4 tasks

Zadri415 reopened this Aug 17, 2026

This comment has been minimized.

Sign in to view

Zadri415 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 17, 2026

This comment has been minimized.

Sign in to view

github-actions Bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 17, 2026

Zadri415 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 17, 2026

This comment has been minimized.

Sign in to view

github-actions Bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 17, 2026

Zadri415 added 2 commits August 18, 2026 22:30

Update index.html

804cd5a

Update script.js

97e0247

Zadri415 added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. 🏕 Priority Mandatory This work is expected labels Aug 18, 2026

cjyuan reviewed Aug 18, 2026

View reviewed changes

		const table = document.getElementById("display");
		const tbody = table.getElementsByTagName("tbody")[0];

Uh oh!

Uh oh!

Conversation

Zadri415 commented Aug 6, 2026

Learners, PR Template

Changelist

Questions

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjyuan commented Aug 17, 2026

Uh oh!

Zadri415 commented Aug 17, 2026 via email

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cjyuan commented Aug 17, 2026

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants