Skip to content
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

West-Midlands-Community | Matthew Law | Module-Data-Flows | WEEK 11 | Book Library #143

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

Conversation

matthewlawpixel
Copy link

@matthewlawpixel matthewlawpixel commented Jan 13, 2025

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

PR is for completing exercises.

Questions

Ask any questions you have for your reviewer.

@matthewlawpixel matthewlawpixel added the Needs Review Participant to add when requesting review label Jan 13, 2025
Copy link

@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.

All the bugs described in the spec have been successfully fixed.

There are also some errors in the index.html.

for (let n = rowsNumber - 1; n > 0; n-- {

// Delete old rows
for (let n = rowsNumber - 1; n > 0; n--) {
Copy link

Choose a reason for hiding this comment

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

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.

Yes, there is a more efficient way to remove all rows (except the elements) in the table. The entire can be cleared in a single operation by setting its innerHTML to an empty string.

delBut.className = "btn btn-warning";
delBut.innerHTML = "Delete";
delBut.addEventListener("clicks", function () {
delButton.id = i + 5;
Copy link

Choose a reason for hiding this comment

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

  • Is the value assigned to this id attribute unique?
  • Is there a need to assign an id attribute to delButton?
  • Is there a need to assign an id attribute to changeBut (at line 62)?
  • Can you think of a more consistent way to name the variables representing the two buttons?

Copy link
Author

@matthewlawpixel matthewlawpixel Jan 17, 2025

Choose a reason for hiding this comment

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

@cjyuan Yes you are right. After reviewing the need for id attributes for the buttons, I determined that they are not necessary I’ve opted to use data-* attributes to manage element associations. I have also changed the variable name for the buttons to be more clear "deleteButton" and "toggleButton"

Comment on lines 76 to 80
delButton.addEventListener("click", function () {
alert(`You've deleted title: ${myLibrary[i].title}`);
myLibrary.splice(i, 1);
render();
});
Copy link

Choose a reason for hiding this comment

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

At the moment when the alert message is displayed, has the book mentioned in the message been deleted yet?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for pointing this out @cjyuan. I have moved the alert message to occur after the book deletion

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jan 16, 2025
@matthewlawpixel
Copy link
Author

Hi @cjyuan if everything is alright here. I'll make this as complete. Thank you for helping me reviewing my code.

@matthewlawpixel matthewlawpixel added the Complete Participant to add when work is complete and review comments have been addressed label Jan 18, 2025
@cjyuan
Copy link

cjyuan commented Jan 18, 2025

yes. all good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Participant to add when work is complete and review comments have been addressed Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants