London | 25-ITP-Sep | Adnaan Abo | Sprint 2 | book library project #344

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

AdnaanA wants to merge 5 commits into CodeYourFuture:main from AdnaanA:book-library

AdnaanA commented Dec 7, 2025 •

edited

Loading

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

Website loads but doesn't show any books. fixed
Error in console when you try to add a book. fixed
It uses the title name as the author name. fixed
Delete button is broken. fied
When I add a book that I say I've read - it saves the wrong answer. fixed

also ran html validation using w3c and fixed all the recommendations.


          Refactor HTML structure, improve form handling, and update README for…

450911b

… clarity

github-actions bot commented Dec 7, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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.

AdnaanA added the Needs Review label

github-actions bot commented Dec 7, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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.

1 similar comment

github-actions bot commented Dec 7, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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 bot removed the Needs Review label

github-actions bot commented Dec 7, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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.

1 similar comment

github-actions bot commented Dec 7, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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.

AdnaanA added the 📅 Sprint 2 label

github-actions bot commented Dec 7, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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.

AdnaanA added the Needs Review label

github-actions bot commented Dec 7, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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 bot removed the Needs Review label

github-actions bot commented Dec 7, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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.

AdnaanA added the Needs Review label

cjyuan reviewed

View reviewed changes

debugging/book-library/index.html Outdated

    
                      <button

                        type="button"

                        class="btn btn-primary mt-3"

                        onclick="addBook()"

Contributor

cjyuan Dec 9, 2025

It's a better practice to "attach event listener via JS" than "mixing JS code in HTML".

Author

AdnaanA Dec 9, 2025

added event listener via JS.

debugging/book-library/script.js Outdated

Comment on lines 32 to 34

    
                  bookTitle.value.trim(),

                  author.value.trim(),

                  pages.value.trim(),

Contributor

cjyuan Dec 9, 2025

It is a good practice to save sanitized or pre-processed values in some variables first
- to avoid calling .trim() repeatedly, and
- to reduce the chance of accidently using the raw value.
The book's page property can still be assigned some invalid page count.

Author

AdnaanA Dec 10, 2025

we can clean the values once and store them in variables: instead of the above code we could do
const titleValue = bookTitle.value.trim();
const authorValue = author.value.trim();
const pagesValue = pages.value.trim();

and we can over come the page being assigned invalid count we can do this

const pagesNumber = Number(pagesValue);
if (!Number.isInteger(pagesNumber) || pagesNumber <= 0) {
alert("Please enter a valid positive page count!");
return;
}

debugging/book-library/script.js Outdated

Comment on lines 52 to 55

    
                // remove all rows except the header

                while (table.rows.length > 1) {

                  table.deleteRow(1);

                }

Contributor

cjyuan Dec 9, 2025

To clear the table before repopulating it, instead of deleting the table rows one by one, can you think of a more efficient way to remove all rows (except the <th>...</th>) in the table?

Author

AdnaanA Dec 9, 2025

we can use tbody.innerHTML = ''; to clear all rows efficiently.

however we have to use const row = tbody.insertRow(); to insert rows into tbody.

debugging/book-library/script.js Outdated

    
                  delBut.id = i + 5;

                  deleteCell.appendChild(delBut);

                  // delete button

                  const deleteCell = row.insertCell(4);

Contributor

cjyuan Dec 9, 2025

Can you think of the pros and cons of these two approaches for creating cells within a row?

Keeping all the cell creation code in one location, like the original code does.
Scattering the cell creation code across different locations, like what you did.

Author

AdnaanA Dec 10, 2025

Keeping all cells created in one block is the best practice.
It is clearer, safer, easier to maintain, and less likely to produce bugs.
while
Scattered cell creation should only be used when absolutely necessary for conditional or dynamic table structures.

cjyuan added Reviewed and removed Needs Review labels

AdnaanA added 4 commits

December 9, 2025 11:38


          Update button functionality to add books and improve event handling

ac6bc70


          Optimize render function to clear table rows more efficiently

ea76889


          Refactor addBook function for improved validation and sanitization

eea5314


          Refactor render function for improved efficiency and clarity in cell …

aadf229

…creation

AdnaanA added the Needs Review label

Contributor

cjyuan commented Dec 10, 2025

Changes look good. Well done.`

cjyuan added Complete and removed Needs Review labels

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

Labels

Complete Reviewed 📅 Sprint 2