Skip to content

West-Midlands ITP-Jan-2025 | Ifeanyi Madubugwu | Module-Data-Flow| Sprint -2| Book Library #189

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.

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Elchacode
Copy link

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

…e from the form. added the function to check if the book is read or not and also added the function to change the status of the book to read or unread.
@Elchacode Elchacode added Needs Review Participant to add when requesting review 📅 Sprint 2 Assigned during Sprint 2 of this module labels Apr 9, 2025
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.

Code is working. I think there are a few improvements you can still make.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I checked the code in index.html at https://validator.w3.org/, it indicated some errors.

Copy link
Author

Choose a reason for hiding this comment

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

Hi CJ

have i fixed my html element and all has passed in https://validator.w3.org/,

Comment on lines 33 to 39
title.value == null ||
title.value == "" ||
title.value.trim() === "" ||
author.value == null ||
author.value.trim() === "" ||
pages.value == null ||
pages.value == ""
pages.value.trim() === ""
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of input validation,

  1. Can .value be null? (Do we need to check someInputElement.value == null?)
  2. What if a user enters an invalid page number in the "pages" input field?

Copy link
Author

Choose a reason for hiding this comment

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

Hi Cj

i have refactor the code and have remove
title.value == null
author.value == null
page.value == null

render();
}
console.log(myLibrary);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this console.log() statement for the app to work?

Copy link
Author

Choose a reason for hiding this comment

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

Hi Cj

i have removed the console.log, i implemented that when i was debugging my code.

Thanks

Comment on lines 61 to 63
for (let n = rowsNumber - 1; n > 0; n-- ) {
table.deleteRow(n);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

Hi Cj

i have refactor the code to clear all row except the header, and also sort myLibrary before rendering.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • With the way the book's title is assigned to an HTML element (line 73), a book with a title containing special character sequence such as <i> can possibly ruin the display.

  • Is the value assigned to the id attribute of changeBut and delButton unique?

  • Is there a need to assign an id attribute to delButton (line 92) or changeBut (line 79)?

  • Can you think of a more consistent way to name the variables representing the two buttons, changeBut and delButton?

Copy link
Author

Choose a reason for hiding this comment

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

Hi Cj

on line 73, i have refactor the code to textContent instead of innerHTML,

i deleted the id attribute for the changeBut and delButton, and i also rename the variable to statusButton and deleteButton

@cjyuan
Copy link
Contributor

cjyuan commented Apr 12, 2025

On a separate note, it's considered good practice to check the boxes in the PR template (to confirm that the listed requirements have been met) and to include a brief description of the PR.

image

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Apr 12, 2025
@Elchacode Elchacode added the Complete Volunteer to add when work is complete and review comments have been addressed label Apr 22, 2025
@cjyuan cjyuan removed the Complete Volunteer to add when work is complete and review comments have been addressed label Apr 22, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Apr 22, 2025

I didn't see any changes made based on my feedback.
If you have made changes on your computer, you need to push your changes to Github.

@Elchacode Elchacode added the Complete Volunteer to add when work is complete and review comments have been addressed label Apr 22, 2025
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 review comments have been addressed Reviewed Volunteer to add when completing a review 📅 Sprint 2 Assigned during Sprint 2 of this module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants