Skip to content

London | 26-ITP-May | Jorvan White | Sprint 2 | Book library - #548

Open
JorvanW wants to merge 17 commits into
CodeYourFuture:mainfrom
JorvanW:book-library
Open

London | 26-ITP-May | Jorvan White | Sprint 2 | Book library#548
JorvanW wants to merge 17 commits into
CodeYourFuture:mainfrom
JorvanW:book-library

Conversation

@JorvanW

@JorvanW JorvanW commented Aug 13, 2026

Copy link
Copy Markdown

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

Created Pull Request from Data Flows Book Library Task

JorvanW added 2 commits August 13, 2026 15:03
… and made sure it can produce error with not inputting author
@github-actions

This comment has been minimized.

@JorvanW JorvanW added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 13, 2026
@JorvanW JorvanW changed the title London | 26-ITP-May | Jorvan White | Data Flows | Book library London | 26-ITP-May | Jorvan White | Sprint 2 | Book library Aug 13, 2026

@cjyuan cjyuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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 13, 2026
@JorvanW

JorvanW commented Aug 16, 2026

Copy link
Copy Markdown
Author

I think I have made some relevant changes following the link given

@JorvanW JorvanW 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 16, 2026
Comment thread debugging/book-library/index.html
Comment thread debugging/book-library/index.html Outdated
<button
type="button"
class="btn btn-primary"
onclick="submit();"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you look up the trade-off between:

  • Assigning event listener in HTML
  • Assigning event listener in JS via .addEventListener()

Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js Outdated
} else {
let book = new Book(title.value, title.value, pages.value, check.checked);
library.push(book);
let book = new Book(title.value, author.value, pages.value, check.checked);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if user enter title as " C++ Programming ", and page count as "30e1", "00030", or "12.345"?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user is no longer able to input anything in page count that isn't a whole positive number

Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js Outdated
@cjyuan 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 16, 2026
@JorvanW

JorvanW commented Aug 17, 2026

Copy link
Copy Markdown
Author

I have made relevant changes from the comments given. Please let me know if there are other issues to work on.

@JorvanW JorvanW 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 17, 2026
@JorvanW

JorvanW commented Aug 17, 2026

Copy link
Copy Markdown
Author

At the time of writing this, I believe github is down and causing some problem with loading

@cjyuan

cjyuan commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Changes are good. Could you also address the remaining three "unresolved" comments left in my previous review? Two of which are related to index.html, and the 3rd one is related to handling of user input.

@cjyuan 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 17, 2026
@JorvanW

JorvanW commented Aug 18, 2026

Copy link
Copy Markdown
Author

Hello, I could be wrong but I have addressed these comments that are not resolved yesterday but because github was down they weren't synced properly. Please let me know if there are still mistakes I have overlooked in case its a fault on my end. Many Thanks

@JorvanW JorvanW 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 18, 2026
@cjyuan

cjyuan commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

You could verify if the changes made on this branch is up to date:

  • Click the "Commits" tab to see all the commits
  • Click the "Files changed" tab to see the content of the changed files.
image

@cjyuan 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 18, 2026
@JorvanW

JorvanW commented Aug 18, 2026

Copy link
Copy Markdown
Author

fixed changes mentioned from comments

@JorvanW JorvanW 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 18, 2026

@cjyuan cjyuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good.

const bookForm = document.getElementById("bookForm");
const bookTableBody = document.getElementById("bookTableBody");

submitButton.addEventListener("click", submit);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could consider placing all code that runs once on page load 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.

This makes it easier to locate and manage all the code that runs once when the app starts.

@cjyuan 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 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants