JS2-Week1-Homework #451
JS2-Week1-Homework #451
Conversation
wilgert
left a comment
wilgert
left a comment
There was a problem hiding this comment.
You did a good job! But try to clean up your code before submitting it. there is some duplication and obsolete code in your css. And my comments are to make you grow, not to make you feel bad!!
Week1/homework/app.js
Outdated
| } | ||
| // the next step is to design a function to create a div => add it to the body => and call the bookLibrary(createTheMainList) | ||
| function page() { | ||
| // create div contains ul & h1 |
There was a problem hiding this comment.
Maybe my explanation was not clear. But the idea is that a comment should tell why the code does something a certain way. Your comments tell the reader what the code does. But the code is already clear enough to tell that by itself.
Week1/homework/app.js
Outdated
| }; | ||
|
|
||
| // Create a function to build a list of books including the required information. | ||
| function bookLibrary(createTheMainList) { |
There was a problem hiding this comment.
The name of the parameter createTheMainList suggests that it is a function. But it is actually an html element. Please rename the parameter to something more appropriate.
Week1/homework/app.js
Outdated
| // declare the variables to loop through the objects.(bookTitles, myBooks, bookCover) | ||
| const theTitle = bookTitles[i]; // loop through the titles (bookTitles object) | ||
| const info = myBooks[theTitle]; // loop through the book information (myBooks object) | ||
| const photoOfCover = bookCover[theTitle]; // loop through the covers (bookCover object) |
There was a problem hiding this comment.
Please read the instructions 1.7 and 1.8 carefully again.
Doing everything in one loop and putting the img in the html in one function is not a bad solution. But it is not what was asked for.
Week1/homework/style.css
Outdated
| } | ||
|
|
||
| p { | ||
| font-family: "Comic Sans MS", cursive, sans-serif; |
There was a problem hiding this comment.
As a side note, and maybe you already know this. But Comic Sans MS is normally not chosen as a font-family on websites. Furthermore I would not use 3 different font-families on a webpage.
Week1/homework/style.css
Outdated
| background-color: rgb(255, 255, 255); | ||
| border-radius: 8px; | ||
| } | ||
| li.item{ |
There was a problem hiding this comment.
All .items are li.items. So you can remove the duplicate code here.
Interesting question: what would the values for display, margin and padding be in the browser with the css that you wrote?
Week1/homework/style.css
Outdated
| margin: auto; | ||
| display: block; | ||
| } | ||
| @media (min-width: 768px) and (max-width: 1024px) { |
There was a problem hiding this comment.
Good that you thought about responsive design!
Week1/homework/style.css
Outdated
| h1 { | ||
| font-size: 3em; | ||
| font-weight: bold; | ||
| color: antiquewhite; |
There was a problem hiding this comment.
Why do you specify a slightly different color when the screen is at this size?
Week1/homework/app.js
Outdated
| author.className = 'author'; | ||
|
|
||
| // add language | ||
| language.innerText = 'language: ' + info.language; |
There was a problem hiding this comment.
Try to be consistent with capitals. language should be Language.
4 participants
No description provided.