NW6 | Orlando_Morales | JS2-Module-Reading-list | Week3#199
NW6 | Orlando_Morales | JS2-Module-Reading-list | Week3#199OrlandoMoralesKuan wants to merge 2 commits intoCodeYourFuture:mainfrom
Conversation
✅ Deploy Preview for cute-gaufre-e4b4e5 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
pseudopilot
left a comment
pseudopilot
left a comment
There was a problem hiding this comment.
@OrlandoMoralesKuan , this is really confident implementation! 👍 I really love it!
I only left two minor comments/questions.
| }, | ||
| ]; | ||
|
|
||
| function renderBooks(book) { |
There was a problem hiding this comment.
As this function renders only one book I would suggest renaming it to renderBook to avoid confusion.
| @@ -4,7 +4,7 @@ | |||
| <meta charset="utf-8" /> | |||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | |||
| <link rel="stylesheet" href="style.css" /> | |||
There was a problem hiding this comment.
I see this line but don't see any .css file here. Was it the part of the task to style the page or not? I'm not really sure but I see one of the requirements:
The page is visually appealing
Maybe you just missed to add it to commit?
| return bookElement; | ||
| } | ||
|
|
||
| function readingList() { |
There was a problem hiding this comment.
One more naming recommendation. Functions should describe action. This allow us to distinguish functions and variables just even by names. I would suggest rename it to buildReadingList or renderReadingList.
2 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.