WM | ITP-MAY-25 | NAHOM MESFIN | Sprint 2 | Book Library #315

nahommesfin77 · 2025-08-19T23:06:37Z

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

I debugged and fixed issues preventing books from displaying. and corrected errors when adding a book. then fixed the title/author mix-up, ensured the delete button works, and corrected the read-status logic. I then implemented validation so incomplete book entries show an alert instead of being added to the list.

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.

nahommesfin77 · 2025-08-20T09:18:05Z

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.

Thanks for the reply, I have checked https://github.com/cjyuan/Module-Data-Flows/blob/book-library-feedback/debugging/book-library/feedback.md and made improvements to the code.

LonMcGregor

Good work on this sprint. It is almost done. I have left some remarks for how you could improve it further

LonMcGregor · 2025-08-20T10:01:31Z

debugging/book-library/script.js

+  const authorValue = author.value.trim();
+  const pagesValue = pages.value.trim();
+
+  if (!titleValue || !authorValue || !pagesValue) {


Are there any values in the form you might want to validate more carefully than just "is there data here"?

yes there are a couple of validation rules i should have added.

line 32

I added a conditon if (!/^[a-zA-Z\s.'-]+$/.test(authorValue) to validate authors value, limiting it to only letters, spaces, periods, apostrophes, and hyphens.

line 38

I added another condition for page number if (!Number.isInteger(pagesNumber) || pagesNumber <= 0) to validate only postive numbers.

LonMcGregor · 2025-08-20T10:03:15Z

debugging/book-library/script.js

+    deleteButton.addEventListener("click", () => {
+      myLibrary.splice(i, 1);  // delete immediately
+      render();                 // update the table
+      alert(`Deleted "${book.title}" successfully.`); // optional confirmation


When you are deleting something like a file on your PC, do you normally expect to be notified after, or is there an interaction that would work that is better than alert?

line 22 - 35

Instead of blocking alerts, i have used a notification function function showNotification(message) which gives a non interrputing visually nicer notification instead of popup alert.

LonMcGregor · 2025-08-20T10:08:43Z

debugging/book-library/index.html

        <label for="title">Title:</label>
        <input
-          type="title"
+          type="text"


Right now you need to manually validate the form. Do you know what change you need to make to the HTML and script to make the browser validate the form inputs automatically?

lines 47 & 56

I have removed the some of the manual validations i used earlier in the script and replaced it with an autotmatic error handler in my html for the inputs pattern="[a-zA-Z\s.'-]+" , required min="1"

cjyuan

The changes you made look good. You can go through my additional suggestions later when you have time.

cjyuan · 2025-08-20T12:06:30Z

debugging/book-library/index.html

+          pattern="[a-zA-Z\s.'-]+"
+          title="Only letters, spaces, apostrophes, periods, or hyphens allowed"


This approach is good but the pattern might be too restrictive.

cjyuan · 2025-08-20T12:14:17Z

debugging/book-library/script.js

+document.getElementById("bookForm").addEventListener("submit", function(event) {
+  event.preventDefault();
+  submit();
+});


Placing code that's to be executed once on page load (lines 3-6, 32-35) together could make locating this kind of code easier.

cjyuan · 2025-08-20T12:15:43Z

debugging/book-library/script.js

+  title.value = "";
+  author.value = "";
+  pages.value = "";
+  check.checked = false;


We could also use .reset() of a form element to clear the form.

cjyuan · 2025-08-20T12:16:41Z

debugging/book-library/script.js

-      myLibrary.splice(i, 1);
-      render();
+    // Delete button
+    const deleteCell = row.insertCell(4);


Can you think of the pros and cons of these two approaches for creating cells within a row?

Keeping all the cell creation code in one location, like the original code does.

Scattering the cell creation code across different locations, like what you did.

nahommesfin77 added 9 commits August 19, 2025 23:04

<tr> remove

6d1db61

submit and console error

74a65bb

title.value to author.value

87c782a

delbutton/clicks typo

13940df

render function condition

87b5187

add validation author

6b150ac

add clickable icon

5b00729

author number validation

6fc08fe

pointer cursor

7ec7ebf

nahommesfin77 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 19, 2025

nahommesfin77 mentioned this pull request Aug 19, 2025

WM | ITP-MAY-25 | Data flows | NAHOM MESFIN nahommesfin77/My-Coursework-Planner#75

Open

nahommesfin77 added 2 commits August 20, 2025 00:22

fix thead

ee53951

remove clickable row

5045390

cjyuan reviewed Aug 20, 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 20, 2025

nahommesfin77 added 5 commits August 20, 2025 09:02

remove render()

f45f0e4

debug validations submit(), render()

76bf57c

remove name, content

4476c43

remove read toggle

714badb

alert

f29e2c0

nahommesfin77 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 20, 2025

LonMcGregor added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Aug 20, 2025

LonMcGregor reviewed Aug 20, 2025

View reviewed changes

create form and require min 1

6547b32

nahommesfin77 added 4 commits August 20, 2025 12:40

remove alert

cff73c9

notification style

85d011f

auto validation

43852c5

remove validation check

e3d4f0e

nahommesfin77 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 20, 2025

cjyuan reviewed Aug 20, 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 20, 2025

illicitonion closed this Sep 1, 2025

		pattern="[a-zA-Z\s.'-]+"
		title="Only letters, spaces, apostrophes, periods, or hyphens allowed"

Uh oh!

WM | ITP-MAY-25 | NAHOM MESFIN | Sprint 2 | Book Library #315

WM | ITP-MAY-25 | NAHOM MESFIN | Sprint 2 | Book Library #315

Uh oh!

Conversation

nahommesfin77 commented Aug 19, 2025

Learners, PR Template

Changelist

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

nahommesfin77 commented Aug 20, 2025

Uh oh!

LonMcGregor 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants