Skip to content

WESTMIDLANDS-ITP-JAN|SEGUN FOLAYAN|DATA FLOWS|BOOK-LIBRARY #208

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 10 commits into
base: main
Choose a base branch
from

Conversation

segunfolayan
Copy link

@segunfolayan segunfolayan commented Apr 20, 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

BOOK LIBRARY PROJECT PR

Questions

Ask any questions you have for your reviewer.

@segunfolayan segunfolayan added the Needs Review Participant to add when requesting review label Apr 20, 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 20, 2025
@jenny-alexander jenny-alexander self-requested a review April 20, 2025 20:38
@@ -76,7 +84,7 @@ function render() {
changeBut.className = "btn btn-success";
wasReadCell.appendChild(changeBut);
let readStatus = "";
if (myLibrary[i].check == false) {
if (myLibrary[i].check == true) {

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 way to rewrite lines 87-91 in order to reduce repetition?

  • The value within myLibrary[i].check will hold either true or false.
  • Can you use a ternary operator to achieve the same result in one line of code

The syntax of the ternary operator is like this:
condition ? trueExpression : falseExpression.
If the condition evaluates to true, the expression after the ? is executed; otherwise, the expression after the : is executed.

Copy link
Author

Choose a reason for hiding this comment

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

readStatus = myLibrary[i].check ? "Yes" : "No";
I have replaced with this line of code.
Thank you

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.

Your PR looks good 👍

Here are a few change requests that will make it even better:

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

  2. The 4 variables on lines 24-27 are currently declared as global variables. However, they are only used within the submit function.
    Can modify your code so that they are no longer global variables and are declared as local variables?
    Making this change means the variables won't be created until they are needed within the submit function. This makes the code cleaner and more maintainable!
    https://www.w3schools.com/js/js_scope.asp

@jenny-alexander jenny-alexander added Needs Review Participant to add when requesting review and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Apr 21, 2025
@segunfolayan
Copy link
Author

segunfolayan commented Apr 21, 2025

Hello,
I will like to let you know that I have implemented the changes as advised.

  1. I have used "const" to declare my variables instead of "let"
  2. I have changed "title", "author", "pages" and "check" to local variables.
    Thank you.

@jenny-alexander jenny-alexander added Complete Volunteer to add when work is complete and review comments have been addressed and removed Needs Review Participant to add when requesting review labels Apr 21, 2025
@segunfolayan
Copy link
Author

Thank you for your prompt and timely review.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants