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

ITP-JAN|LONDON|FOROGH AGHAEIYARIJANI|BOOK LIBRARY PROJECT|WEE 2|problem solution #174

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

Conversation

Forogh-Aghaeiyarijani
Copy link

@Forogh-Aghaeiyarijani Forogh-Aghaeiyarijani commented Mar 27, 2025

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

Added logic to display a table of books with the ability to add/remove/update read status.

Cleaned up variable declarations by replacing let with const where appropriate.

Ensured form resets on successful book submission.

Questions

Ask any questions you have for your reviewer.

@Forogh-Aghaeiyarijani Forogh-Aghaeiyarijani added the Needs Review Participant to add when requesting review label Mar 27, 2025
@jenny-alexander jenny-alexander added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Participant to add when requesting review labels Apr 5, 2025
@jenny-alexander jenny-alexander self-requested a review April 5, 2025 22:49
Copy link

@jenny-alexander jenny-alexander left a comment

Choose a reason for hiding this comment

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

The fixes you implemented work well and it now meets all of the requirements of this task. Here are a few things you should implement in order to make your code even better!

  • Can you review all of the places that you declared a variable with 'let'? In general, you will mostly always use const (and this should be your default). If you need to reassign a value to the variable, then you would use let. Here is a video you can watch: https://youtu.be/RE6qf3As-XU?si=_YaiEPeBC5Cx_j56
  • The changelist portion of your pull request is currently empty. This is an important part that helps other engineers understand what you've modified. Could you please add a brief description of your changes in the changelist section? A simple summary is good enough!
  • Can you check off the list of requirements within the 'Self checklist' part of the PR? This is a good way to review what you need to do before submitting your PR. :)

⭐ This is a recommendation (you don't have to make this change but I highly recommend it). If you look within the package.json file, you'll see that there is a script named format. If you run this script from the terminal, it will format your code to have a consistent style. The file .prettierrc defines the formatting that will done if you run this script.

@jenny-alexander jenny-alexander added Reviewed Volunteer to add when completing a review and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Apr 5, 2025
@Forogh-Aghaeiyarijani
Copy link
Author

Thank you so much for reviewing my code.I did it

Copy link

@jenny-alexander jenny-alexander left a comment

Choose a reason for hiding this comment

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

Most of the changes were implemented. Just missing this one:
Can you explain your changes within the 'Change List' part of the PR?
Screenshot 2025-04-05 at 11 12 42 PM

@@ -1,45 +1,34 @@
let myLibrary = [];

Choose a reason for hiding this comment

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

This is an example of a variable that should be declared as a const.
There are other variables within script.js that are declared as let but should be const.
➡️ Can you find them and change them?
Using const prevents accidental reassignment of the variable and makes your code easier to read.

Choose a reason for hiding this comment

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

yes,I did it.

@Forogh-Aghaeiyarijani
Copy link
Author

Changelist:

Added logic to display a table of books with ability to add/remove/update read status.

Cleaned up variable declarations by replacing let with const where appropriate.

Ensured form resets on successful book submission.

Copy link

@jenny-alexander jenny-alexander left a comment

Choose a reason for hiding this comment

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

The change list portion still needs to be updated. Please update directly within the changelist section!

Choose a reason for hiding this comment

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

There are 2 other let variables that should be changed to const since they are not being reassigned. Can you find them and make the change please?

Choose a reason for hiding this comment

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

I did it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants