-
-
Notifications
You must be signed in to change notification settings - Fork 155
London | May-2025 | Ikenna Agulobi | Data Groups | sprint 3 | Reading List #704
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
base: main
Are you sure you want to change the base?
Conversation
margin-top: 50px; | ||
} | ||
|
||
.read{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the colour for read class should be green to pass the test!
background-color: red; | ||
} | ||
|
||
.not-read{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the colour for not-read class should be red to pass the test! - check the reading-list.png
for expected result.
@@ -21,3 +21,34 @@ const books = [ | |||
}, | |||
]; | |||
|
|||
function readingList(books) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rename the function starting with verb, and describe the action more clearly?
if (books[i].alreadyRead) { | ||
li.classList.add("read"); // If true, add the 'read' class | ||
} else { | ||
li.classList.add("not-read"); // If false, add the 'not-read' class | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works fine, but this lines of code can be written in one line using a ternary operator. Can you refactor the code?
function readingList(books) { | ||
const readingListUl = document.getElementById("reading-list"); | ||
|
||
for (let i = 0; i < books.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you refactor a code to use forEach method with books array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you run the tests? There are two failing tests. Please take a look. Otherwise, everything else works as intended!
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
This PR includes my completed Reading List app for Sprint 3 - Data Groups module.
Questions
Ask any questions you have for your reviewer.