Skip to content

Sheffield | ITP-Jan-26 | Hayriye Saricicek | Sprint 2 | Book Library#457

Closed
mshayriyesaricicek wants to merge 5 commits intoCodeYourFuture:mainfrom
mshayriyesaricicek:Sprint-2-book-library
Closed

Sheffield | ITP-Jan-26 | Hayriye Saricicek | Sprint 2 | Book Library#457
mshayriyesaricicek wants to merge 5 commits intoCodeYourFuture:mainfrom
mshayriyesaricicek:Sprint-2-book-library

Conversation

@mshayriyesaricicek
Copy link
Copy Markdown

@mshayriyesaricicek mshayriyesaricicek commented Apr 19, 2026

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

I made changes to make the drop down form work and upload the book details to the book list. I put an Enter button on the form. If all books are deleted it will automatically load up the preloaded books. If books are added it will save the information so it is available on reload. All information is in the correct place.

@github-actions

This comment has been minimized.

@mshayriyesaricicek mshayriyesaricicek added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Data-Flows The name of the module. labels Apr 19, 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.

Code is not properly indented, making it hard to read. Can you fix the Indention?

Comment thread debugging/book-library/index.html
Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js Outdated
@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 19, 2026
@mshayriyesaricicek mshayriyesaricicek added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Apr 21, 2026
Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js
Comment thread debugging/book-library/script.js Outdated

render();

this.reset();
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 seems quite advanced. Are you familiar with the meaning of this in JS?

In Piscine, you may be asked something like this:

  • If you were not allowed to use this, how would you rewrite this.reset()?

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.

If I couldn’t use this.reset(), I would manually reset each form field by setting their values back to empty. For example: document.getElementById("title").value = "";
document.getElementById("author").value = "";
document.getElementById("pages").value = "";
document.getElementById("check").checked = false;

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.

You can use any code as long as can explain the code when asked. In Piscine's interview, the interviewer may randomly pick a piece of code from your project and ask you to explain it.

Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js Outdated
Comment thread debugging/book-library/script.js
@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 21, 2026
@mshayriyesaricicek mshayriyesaricicek added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Apr 23, 2026
render();
}

let newBook = new Book(title, author, pages, check);
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Apr 23, 2026

Choose a reason for hiding this comment

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

Why do you choose to keep the pages as string? It is not wrong, just unusual.

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.

ive changed it now

Comment thread debugging/book-library/script.js Outdated
Comment on lines 130 to 135
alert(`You've deleted title: ${myLibrary[i].title}`);
myLibrary.splice(i, 1);

saveLibrary();

render();
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 alert message is shown before the book is actually deleted; the deletion only occurs after the alert dialog is dismissed. This introduces a risk that the operation may not complete (e.g., if the user closes the browser before dismissing the alert).

It’s better to display a confirmation message only after the associated operation has successfully completed.

Where should you place the alert() function call?

Copy link
Copy Markdown
Author

@mshayriyesaricicek mshayriyesaricicek Apr 23, 2026

Choose a reason for hiding this comment

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

When I load it up on live server and delete a book the delete message comes up after deleting the book not before but I have moved it to a more suitable position.

Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Apr 23, 2026

Choose a reason for hiding this comment

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

alert() blocks execution of subsequent statements. To simulate the behavior, you can try this:

      alert(`You've deleted title: ${myLibrary[i].title}`);
      throw new Error("simulate something wrong happening ...");
      myLibrary.splice(i, 1);

      saveLibrary();

      render();

The error will only throw after you close the alert dialog box.

@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 23, 2026
@mshayriyesaricicek mshayriyesaricicek added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Apr 23, 2026
@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Apr 23, 2026

Changes look good.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Apr 23, 2026
@illicitonion
Copy link
Copy Markdown
Member

Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it.

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.

3 participants