Glasgow | 26-ITP-Jan| Martin McLean |Sprint 2|Book Library by mjm-git185 · Pull Request #448 · CodeYourFuture/Module-Data-Flows

mjm-git185 · 2026-04-16T11:09:42Z

Changelist

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
my coursework

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.

…f render function

cjyuan · 2026-04-17T12:50:35Z

According to https://validator.w3.org/, there are errors in your index.html. Can you fix these errors?

Thanks this site was very helpful

cjyuan · 2026-04-17T12:50:56Z

@@ -1,100 +1,101 @@
 let myLibrary = [];


Safer to declare this using const.

cjyuan · 2026-04-17T12:57:17Z

+  const titleInput = document.getElementById("title").value;
+  const authorInput = document.getElementById("author").value;
+  const pagesInput = document.getElementById("pages").value;
+  const checkInput = document.getElementById("check").checked;
+  if (titleInput == "" || pagesInput == "" || authorInput == "") {


.value is a string. As such, pagesInput could be a number in scientific format (e.g. "3e2").

What is an acceptable range of page count?

Should the app be accepting a string containing only spaces (e.g. " ") as the title or author of the book?

cjyuan · 2026-04-17T12:58:20Z

-    return false;
+    return true;


Why return true when some of the input fields is empty?

cjyuan · 2026-04-17T12:59:40Z

+    //clear the input values
  }


What is this comment about?

cjyuan · 2026-04-17T13:05:02Z

    authorCell.innerHTML = myLibrary[i].author;
    pagesCell.innerHTML = myLibrary[i].pages;

+    //deleteCell.innerHTML = button[i]


You should delete all unnecessary code instead of keeping them in a comment.

cjyuan · 2026-04-17T13:07:39Z

+    myLibrary[i].check == true 
+      ? (readStatus = "Yes") 
+      : (readStatus = "No");
+


Normally way to use ? : is:

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

condition ? v1 : v2 is an expression. It evaluates to v1 if condition is true, and to v2 if condition is false.

.check is a boolean value. Comparing it to true is optional.

cjyuan · 2026-04-17T13:09:09Z

+      myLibrary[i].check == true
+        ? (myLibrary[i].check = false)
+        : (myLibrary[i].check = true);


This can be simplified into
myLibrary[i].check = !myLibrary[i].check;

cjyuan · 2026-04-17T13:09:52Z

-    delBut.innerHTML = "Delete";
-    delBut.addEventListener("clicks", function () {
+    const button = document.createElement("button");
+    button.id = i;


Lines 74, 94:

Are the values assigned to these id attributes unique?

Is there any need to assign an id attribute to either buttons?

cjyuan · 2026-04-17T13:10:15Z

      alert(`You've deleted title: ${myLibrary[i].title}`);
      myLibrary.splice(i, 1);
      render();


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.

librery should work as expected now

3d8b1b7

This comment has been minimized.

Sign in to view

file fixed

b0f2a10

mjm-git185 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 16, 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 Apr 16, 2026

cjyuan reviewed Apr 16, 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 Apr 16, 2026

mjm-git185 added 2 commits April 17, 2026 12:56

made a start on the refactor stuck on how to clear the table at top o…

c2e4d17

…f render function

i think i got it

cd3f7dd

mjm-git185 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 17, 2026

cjyuan reviewed Apr 17, 2026

View reviewed changes

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

12 of the 13 issuse adressed haveing some trouble with the delete order

6757af6

mjm-git185 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 17, 2026

Uh oh!

Conversation

mjm-git185 commented Apr 16, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants