Birmingham | ITP-Sep-2025 |Ahmad Ehsas | Sprint 2 | Book library #339

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

ahmadehsas wants to merge 7 commits into CodeYourFuture:main from ahmadehsas:feature/book-library

ahmadehsas commented Dec 5, 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

Changelist

I have completed all the required tasks.

Questions

github-actions bot commented Dec 5, 2025

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data flows) doesn't match expected format (example: 'Sprint 2', without quotes)

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.

ahmadehsas changed the title ~~Birmingham | ITP-Sep-2025 |Ahmad Ehsas | Data flows | book library~~ Birmingham | ITP-Sep-2025 |Ahmad Ehsas | Data Flows | book library

github-actions bot commented Dec 5, 2025

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Flows) doesn't match expected format (example: 'Sprint 2', without quotes)

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.

ahmadehsas changed the title ~~Birmingham | ITP-Sep-2025 |Ahmad Ehsas | Data Flows | book library~~ Birmingham | ITP-Sep-2025 |Ahmad Ehsas | Data-flows | book library

github-actions bot commented Dec 5, 2025

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data-flows) doesn't match expected format (example: 'Sprint 2', without quotes)

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.

ahmadehsas changed the title ~~Birmingham | ITP-Sep-2025 |Ahmad Ehsas | Data-flows | book library~~ Birmingham | ITP-Sep-2025 |Ahmad Ehsas | data flows | book library

github-actions bot commented Dec 5, 2025

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (data flows) doesn't match expected format (example: 'Sprint 2', without quotes)

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.

ahmadehsas changed the title ~~Birmingham | ITP-Sep-2025 |Ahmad Ehsas | data flows | book library~~ Birmingham | ITP-Sep-2025 |Ahmad Ehsas | Book library

github-actions bot commented Dec 5, 2025

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Wrong number of parts separated by |s

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.

ahmadehsas changed the title ~~Birmingham | ITP-Sep-2025 |Ahmad Ehsas | Book library~~ Birmingham | ITP-Sep-2025 |Ahmad Ehsas | Data flows

github-actions bot commented Dec 5, 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.

ahmadehsas changed the title ~~Birmingham | ITP-Sep-2025 |Ahmad Ehsas | Data flows~~ Birmingham | ITP-Sep-2025 |Ahmad Ehsas | Sprint 2 | Book library

ahmadehsas added the Needs Review label

cjyuan reviewed

View reviewed changes

Contributor

cjyuan left a comment

Branch is not clean. Please revert all unrelated changes.
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 added Reviewed and removed Needs Review labels

ahmadehsas added Needs Review and removed Reviewed labels

cjyuan reviewed

View reviewed changes

Contributor

cjyuan left a comment

Branch is still not clean. Please revert all unrelated changes.

debugging/book-library/script.js Outdated

Comment on lines 23 to 27

    
                if (!title.value.trim() || !author.value.trim() || !pages.value.trim()) {

                  alert("Please fill all fields!");

                  return false;

                } else {

                  let book = new Book(title.value, title.value, pages.value, check.checked);

                  library.push(book);

                  render();

                  return;

                }

                let book = new Book(title.value.trim(), author.value.trim(), pages.value, check.checked);

Contributor

cjyuan Dec 7, 2025

Without proper validation or pre-processing, the book could be assigned an invalid "number of pages".
Some of the function calls are unnecessarily repeated. To improve performance, consider storing the results of these function calls in variables and referencing those variables instead.

Author

ahmadehsas Dec 8, 2025

Updated the addBook function with improved validation and optimised input handling

Added validation for the "number of pages" field to ensure it is a positive number.
Optimised the addBook function by storing input values in variables to avoid redundant calls.
Updated the form in index.html to include placeholders and proper input attributes for a better user experience.

debugging/book-library/script.js

Comment on lines 9 to 12

    
                if (myLibrary.length === 0) {

                  let book1 = new Book("Robinson Crusoe", "Daniel Defoe", "252", true);

                  let book2 = new Book("The Old Man and the Sea", "Ernest Hemingway", "127", true);

                  myLibrary.push(book1, book2);

Contributor

cjyuan Dec 7, 2025

While storing page counts as string does not affect this simple app, it is a bad practice to represent same kind of values in different data type.

Author

ahmadehsas Dec 8, 2025

Update the populateStorage function to store page counts as numbers instead of strings

Changed page counts in the sample books to use numbers for consistency.
Ensures better data representation and avoids potential issues with numerical operations.

cjyuan added Reviewed and removed Needs Review labels

Author

ahmadehsas commented Dec 8, 2025

The branch is clean. And, everything is up to date.

ahmadehsas added Needs Review and removed Reviewed labels

Contributor

cjyuan commented Dec 9, 2025

Changes to the code are good.

Branch is still not clean, You can see all the changed files in the "Changed files" tab of this PR.

cjyuan added Reviewed and removed Needs Review labels

Author

ahmadehsas commented Dec 9, 2025

When I checked the branch's status, everything was up to date. Can you give me more information about why the branch is not clean?

Contributor

cjyuan commented Dec 9, 2025

Your branch is not clean because it contains unrelated changes (changes unrelated to Book Library exercise) in the Sprint-1 folder. The are unrelated changes in the Sprint-1 folder because you didn't create this branch from main (you created this branch from feature/destructuring).

So the make this branch clean, you have to revert the changes made in the Sprint-1 folder.

Author

ahmadehsas commented Dec 9, 2025

I am very confused about how to revert the changes. Can you guide me or make a Google Meet call in your free time?

ahmadehsas added Needs Review and removed Reviewed labels

Contributor

cjyuan commented Dec 10, 2025

Since you created the branch from feature/destructuring instead of from main, you can use "git rebase" to rebase branch onto main. Here are the instructions:

In your case: B1 is feature/destructuring and B2 is feature/book-library.

Important:

You need to execute the commands within your cloned repository.
You may want to backup your files before trying these commands

1. Open Your Cloned Repository in VSCode and Start a Terminal in VSCode.

VSCode will start the terminal in the top-level folder of the current project.

2. Switch to the branch you want to rebase (`B2`)

git switch B2

Note:

You can check which branch is the current branch via the command git branch (to list all branches with current branch highlighted)

3. Rebase `B2` from `B1` onto `main`

git rebase --onto main B1 B2

For more details about this command, ask an AI tool or see
https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase#:~:text=interactive%20rebase%20display-,Advanced%20rebase%20application,-The%20command%20line

4. Update (and Overwrite) your files in the remote branch B2 (on Github)

While you are in branch B2 and you have verified that it has been successfully rebased, execute the following command to update the remote branch (on GitHub):
git push --force origin

Contributor

cjyuan commented Dec 10, 2025

Alternatively, you can manually replace those "unrelated changed files" by the same files in CYF's main, then make a new commit.

ahmadehsas added 7 commits

December 10, 2025 14:24


          Fix syntax error.

f4be6a3


          Fix error in console when trying to add book.

01c4811


          Fix the delete button error.

de116f7


          Fix the error.

8c4795e


          Update index.html and script.js code.

7e65169


          Update addBook function with improved validation and optimized input …

6e184cf

…handling.


          Update the function populateStorage ro store the page counts as numbe…

d6df415

…rs instead of strings.

ahmadehsas force-pushed the feature/book-library branch from fa7a257 to d6df415 Compare

December 10, 2025 14:25

Author

ahmadehsas commented Dec 10, 2025

Thank you for the guidance.
I rebased the feature/book-library onto the main using
git rebase --onto main feature/destructuring feature/book-library
and force-pushed the updated branch.
The PR now only contains Book Library–related changes.

Contributor

cjyuan commented Dec 10, 2025

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