Skip to content

London | ITP-May-25 | Houssam Lahlah | Sprint 3 | Reading List #750

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

HoussamLh
Copy link

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

What I did:

  • Implemented a reading list that dynamically displays a list of books using JavaScript.

  • Displayed book title and author, and added book cover images.

  • Highlighted books that are already read by changing their background color.

  • Structured the page using semantic HTML (ul, li, p, img).

  • Followed best practices for DOM manipulation and accessibility.

## Questions

  • Do you think the logic for marking books as read is clear and maintainable?

  • Is there anything I could improve in how I structure the list items or apply styles?

  • Is my use of JavaScript efficient and clear?

- Changed console.log to use address.houseNumber instead of address[0]
- Ensures correct display of house number in output
- Replaced invalid for...of loop on object with Object.values()
- Ensures all property values of author are logged without errors
- Replaced object string output with ingredients joined by newline
- Ensures each ingredient is printed on its own line as intended
@HoussamLh HoussamLh added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 6, 2025
@LonMcGregor LonMcGregor added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 11, 2025
Copy link

@LonMcGregor LonMcGregor left a comment

Choose a reason for hiding this comment

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

Good work on this sprint, your reading list work well.

For your questions:

  1. Yes, I think this is very readable and should be easy to maintain
  2. One change you might want to make that could follow best practice more would be to give each element a class, and then style that class in a css file, rather than setting the colour directly in javascript. Do you want to attempt this?
  3. Yes, this javascript looks efficient and clear

@LonMcGregor LonMcGregor added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Volunteer to add when completing a review with trainee action still to take.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants