Skip to content

Brussels | ITP-2025-1 | Daniela Fajardo | Week 11 | Book library#66

Open
ldfajardo10-tech wants to merge 5 commits intoHackYourFutureBelgium:mainfrom
ldfajardo10-tech:book-library
Open

Brussels | ITP-2025-1 | Daniela Fajardo | Week 11 | Book library#66
ldfajardo10-tech wants to merge 5 commits intoHackYourFutureBelgium:mainfrom
ldfajardo10-tech:book-library

Conversation

@ldfajardo10-tech
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

Some bugs fixed and a new feature added to clear the form once is submited.

Copy link
Copy Markdown
Collaborator

@EL1VAS EL1VAS left a comment

Choose a reason for hiding this comment

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

Helooo! Some changes in Book library index.html:
in line 35 type="title" should be type="text" as title is not a valid type. Same in line 43.
At line 67 value="Submit" is not necassery as you define it as Submit at line 69 (but that's minor).

Changes on script.js of Book library:
Line 10: nice catch correcting Robinson from Robison, no one else that I checked saw that one :)
Line 29-33 Nice clearing the form by submit!
Line 39 after you can add the author conditions so user is obliged to add name, pages AND author of the book: author.value == null || author.value == "".

I see you have the prep files there too, I won't be reviewing those.

@operatingsystem05
Copy link
Copy Markdown
Collaborator

Thank you for your review @EL1VAS. Make sure to implement the changes necessary @ldfajardo10-tech, good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants