Skip to content

London | 26-ITP-Jan | Carlos Abreu |Sprint 2 | Book Library#449

Open
carlosyabreu wants to merge 3 commits intoCodeYourFuture:mainfrom
carlosyabreu:feature/book-library
Open

London | 26-ITP-Jan | Carlos Abreu |Sprint 2 | Book Library#449
carlosyabreu wants to merge 3 commits intoCodeYourFuture:mainfrom
carlosyabreu:feature/book-library

Conversation

@carlosyabreu
Copy link
Copy Markdown

@carlosyabreu carlosyabreu commented Apr 16, 2026

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

Book library's PR

@carlosyabreu carlosyabreu added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 16, 2026
cjyuan
cjyuan reviewed Apr 16, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan left a comment

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 Apr 16, 2026
@carlosyabreu
Copy link
Copy Markdown
Author

carlosyabreu commented Apr 17, 2026 •
edited
Loading

@cjyuan
Following the general feedback link you provided, I made minor HTML fix.
In index.html, I changed the author input from type="author" to type="text" and checked required is present.
Also, I added min="1" to the pages input.


To truly isolate scope I did:
Remove onclick="submit();" from the submit button.
Add type="module" to the script tag.
Inside script.js, I used document.querySelector(...).addEventListener("click", submit).
That change in script.js made it possible to work with type="module".
Therefore, the script no longer rely on a global submit() function. Instead, we attach an event listener inside the module.
We refactored the coding (script) and made minor changes in index.html and pushed it again to GitHub.
I checked the HTML code on (https://validator.w3.org/) it passes the test with no errors.

@carlosyabreu carlosyabreu added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 17, 2026
cjyuan
cjyuan reviewed Apr 17, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Changes look good.

Comment thread debugging/book-library/script.js
const readCheckbox = document.getElementById("check");
const submitBtn = document.getElementById("submitBtn");

let myLibrary = [];
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Apr 17, 2026

Choose a reason for hiding this comment

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

Safer to declare this using const, as myLibrary should not be reassigned a new value.

Comment thread debugging/book-library/script.js
library.push(book);
render();
function submit(event) {
event.preventDefault(); // prevent any form-like behavior
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Apr 17, 2026

Choose a reason for hiding this comment

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

You didn't use any form in HTML though.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Apr 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjyuan cjyuan cjyuan left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@carlosyabreu @cjyuan