Skip to content

London | 26-ITP-May | Russom Gebremeskel | Sprint 2 | Book Library - #562

Open
russom-g wants to merge 40 commits into
CodeYourFuture:mainfrom
russom-g:feature/book-library
Open

London | 26-ITP-May | Russom Gebremeskel | Sprint 2 | Book Library #562
russom-g wants to merge 40 commits into
CodeYourFuture:mainfrom
russom-g:feature/book-library

Conversation

@russom-g

@russom-g russom-g commented Aug 17, 2026

Copy link
Copy Markdown

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

Bug fixed. Dead code removed.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

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

This comment has been minimized.

@github-actions 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
@russom-g russom-g changed the title London | 26-ITP-May | Russom Gebremeskel | Book Library Project London | 26-ITP-May | Russom Gebremeskel | Book Library Aug 17, 2026
@github-actions

This comment has been minimized.

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

This comment has been minimized.

@github-actions 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
@russom-g russom-g changed the title London | 26-ITP-May | Russom Gebremeskel | Book Library London | 26-ITP-May | Russom Gebremeskel | Book Library Aug 17, 2026
@russom-g russom-g added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 17, 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.

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 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
@russom-g

Copy link
Copy Markdown
Author

Thanks for the feedback. Now a setup function is implemented. Both populateStorage and render function are wrapped inside the function.

@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.

@russom-g

Copy link
Copy Markdown
Author

Files that shouldn't be here are removed. I also have made some improvements following the requirement document. Thanks

@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.

HTML code could use some more improvements. I will mark this PR as "Complete" first.

Comment on lines 46 to +53
<input
type="number"
class="form-control"
id="pages"
name="pages"
min="1"
required
/>
>

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.

The browser checks the input elements against the specified constraints only when a user submits a form.
Without <form>, the browser won't check the input against the constraints such as required and min="1".

Comment on lines 54 to +59
<label class="form-check-label">
<input
type="checkbox"
class="form-check-input"
id="check"
value=""
/>Read
>

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.

This checkbox is not showing.

The issue is related to Bootstrap 4.4.1. Could you use AI to find a way to fix the issue? Mentioning "Bootstrap 4.4.1" might help.

this.author = author;
this.pages = pages;
this.check = check;
if (isNaN(pages) || pages <= 0) {

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 check if pages is a whole number instead.

@cjyuan

cjyuan commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

note: You shouldn't have deleted the .gitignore that was in the original repo.

@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. labels Aug 21, 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