Skip to content

London | 26-ITP-January | Laura C | Sprint 2 | Book Library#401

Open
lauracs24 wants to merge 4 commits intoCodeYourFuture:mainfrom
lauracs24:feature/book-library
Open

London | 26-ITP-January | Laura C | Sprint 2 | Book Library#401
lauracs24 wants to merge 4 commits intoCodeYourFuture:mainfrom
lauracs24:feature/book-library

Conversation

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

  • Fixed issue where books were not displayed on page
  • Fixed error when adding a new book to the library
  • Corrected author field being incorrectly assigned
  • Fixed delete button
  • Fixed read status displaying incorrect values
  • Improved input validation using trim()
  • Cleared form fields after submission to improve user experience

Questions

Are there any further improvements I could make to the structure or readability of this code?

@lauracs24 lauracs24 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 4, 2026
@lauracs24 lauracs24 added the Module-Data-Flows The name of the module. label Apr 4, 2026
@lauracs24 lauracs24 moved this to Awaiting Approval🚀 in Coursework Planner Apr 4, 2026
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 also 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.

title.value.trim() === "" ||
author.value.trim() === "" ||
pages.value === "" ||
Number(pages.value) <= 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.

  • Rejecting negative page count is good. Perhaps the alert message should be rephrased accordingly?

  • What type of numbers should page count be?

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.

I updated validation so page count must be a positive whole number and improved the alert message

@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 4, 2026
@lauracs24
Copy link
Copy Markdown
Author

Thanks for the feedback. I reviewed the general feedback document and applied the relevant improvements
I:

  • improved input validation and sanitisation using trim()
  • ensured page count is stored and validated as a number
  • updated variable names to make it clear they reference DOM elements
  • removed unnecessary or duplicate logic
    etc

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

cjyuan commented Apr 7, 2026

Changes look good. Well done.

@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 7, 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. Module-Data-Flows The name of the module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants