Skip to content

Manchester| 25-ITP-SEP| Fithi Teklom| Sprint 2 | Book Library #361

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.

Sign up for GitHub

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

Jump to bottom
Open
Fithi-Teklom wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Manchester| 25-ITP-SEP| Fithi Teklom| Sprint 2 | Book Library #361

Fithi-Teklom wants to merge 11 commits into from

Conversation

@Fithi-Teklom
Copy link

@Fithi-Teklom Fithi-Teklom commented Dec 16, 2025

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

@Fithi-Teklom Fithi-Teklom added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 16, 2025
@github-actions
Copy link

github-actions bot commented Dec 16, 2025

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above.

@github-actions github-actions bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 16, 2025
@github-actions
Copy link

github-actions bot commented Dec 16, 2025

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above.

@Fithi-Teklom Fithi-Teklom changed the title Manchester| 25-ITP-SEP| Fithi Teklom| Sprint-1 | Book Library Manchester| 25-ITP-SEP| Fithi Teklom| Sprint 2 | Book Library Dec 16, 2025
@Fithi-Teklom Fithi-Teklom added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 17, 2025
cjyuan
cjyuan reviewed Dec 17, 2025
View reviewed changes
Copy link
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 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.

@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 Dec 17, 2025
@Fithi-Teklom Fithi-Teklom 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 Dec 18, 2025
cjyuan
cjyuan reviewed Dec 18, 2025
View reviewed changes
debugging/book-library/script.js Outdated
Comment on lines 30 to 33
if (!title.value.trim() || !author.value.trim() || !pages.value.trim()) {
alert("Please fill all fields!");
return false;
}
Copy link
Contributor

@cjyuan cjyuan Dec 18, 2025

Choose a reason for hiding this comment

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

Indentation is off.

debugging/book-library/script.js Outdated
library.push(book);
render();
}
let book = new Book(title.value.trim(), author.value.trim(), pagesNumber, check.checked);
Copy link
Contributor

@cjyuan cjyuan Dec 18, 2025

Choose a reason for hiding this comment

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

  • Could consider saving preprocessed and sanitised input in some variables first to

    • improve performance
    • to avoid accidently using raw input later

  • Not all integers are valid number of pages.

debugging/book-library/script.js
Comment on lines 66 to 67
let pagesCell = row.insertCell(2);
let wasReadCell = row.insertCell(3);
Copy link
Contributor

@cjyuan cjyuan Dec 18, 2025

Choose a reason for hiding this comment

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

If a variable is not going to be reassigned a value, it is best practice to declare the variable using const.

debugging/book-library/script.js Outdated
Comment on lines 78 to 85
let readStatus = "";
if (myLibrary[i].check == false) {
if (myLibrary[i].check == true) {
readStatus = "Yes";
} else {
readStatus = "No";
}
changeBut.innerText = readStatus;
toggleReadBtn.innerText = readStatus;
Copy link
Contributor

@cjyuan cjyuan Dec 18, 2025

Choose a reason for hiding this comment

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

This could be a good opportunity to practice using the ? : conditional operator. Can you rewrite the code on lines 79–85 as a single statement?

debugging/book-library/script.js Outdated
deleteBtn.dataset.index = i;
deleteCell.appendChild(deleteBtn);
deleteBtn.className = "btn btn-warning";
deleteBtn.innerHTML = "Delete";
Copy link
Contributor

@cjyuan cjyuan Dec 18, 2025

Choose a reason for hiding this comment

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

Why assign text to a DOM element via three different properties?

  • 'textContent` on line 71
  • innerText on line 85
  • innerHTML on line 97

debugging/book-library/script.js Outdated
changeBut.className = "btn btn-success";
wasReadCell.appendChild(changeBut);
let toggleReadBtn = document.createElement("button");
toggleReadBtn.dataset.index = i;
Copy link
Contributor

@cjyuan cjyuan Dec 18, 2025

Choose a reason for hiding this comment

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

What is the purpose of this statement? Where is the assigned value used in your code?

debugging/book-library/script.js Outdated
Comment on lines 57 to 59
for (let n = rowsNumber - 1; n > 0; n-- ) {
table.deleteRow(n);
}
Copy link
Contributor

@cjyuan cjyuan Dec 18, 2025

Choose a reason for hiding this comment

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

Could consider using a more efficient approach to clear the table.

@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 Dec 18, 2025
@Fithi-Teklom Fithi-Teklom 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 Dec 19, 2025
cjyuan
cjyuan reviewed Dec 19, 2025
View reviewed changes
Copy link
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.

The most recent changes look good, but they may have introduced a bug in the app.

Can you try adding a new book through the UI and see if the book list is still rendered correctly?

@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 Dec 19, 2025
@Fithi-Teklom Fithi-Teklom 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 Dec 19, 2025
@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 Dec 19, 2025
@Fithi-Teklom Fithi-Teklom mentioned this pull request Dec 20, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjyuan cjyuan cjyuan left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Fithi-Teklom @cjyuan