Glasgow | 25-ITP-SEP | Hanna Mykytiuk| Sprint 3| Reading-list #857
Conversation
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
HannaOdud
added
📅 Sprint 3
AbogeJr
added
Review in progress
AbogeJr
left a comment
AbogeJr
left a comment
There was a problem hiding this comment.
Good work overall 👌. Please check out the comments.
| const title = book.title; | ||
| const author = book.author; | ||
| const alreadyRead = book.alreadyRead; | ||
| const bookCoverImage = book.bookCoverImage; |
There was a problem hiding this comment.
Good job with this.
Just a few minor improvements you could make:
- Seeing as the value of
bookwon't change inside ourforloop, do you think there is a better way to initialize it. - While this will work fine, can you find out if there is a cleaner way to unpack object properties into variables.
| const author = book.author; | ||
| const alreadyRead = book.alreadyRead; | ||
| const bookCoverImage = book.bookCoverImage; | ||
| const readingListDom = document.getElementById("reading-list") |
There was a problem hiding this comment.
I don't think we need to initialize the readingListDom object inside this block. Could you look into why that would be problematic?
| let authorDom = document.createElement("p"); | ||
| authorDom.innerText = author; | ||
| let imageDom = document.createElement("img"); | ||
| imageDom.src = bookCoverImage; |
There was a problem hiding this comment.
Nice. Although img tags typically include a certain attribute that you haven't added here. The attribute is meant to improve accessibility and it is also displayed in place of the image in case it does not load. Please look into it.
| .book{ | ||
| background-color: red; | ||
| } | ||
| .alreadyRead{ |
There was a problem hiding this comment.
While this will work fine, css classes are typically not written in camelCase.
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <link rel="stylesheet" href="style.css" /> | ||
| <title>Title here</title> | ||
| <title>Reading list app</title> |
AbogeJr
added
Reviewed
3 participants
Learners, PR Template
Self checklist
Changelist
Reading-list
Questions
No questions.