Skip to content
This repository was archived by the owner on May 14, 2024. It is now read-only.

Conversation

@rabrad
Copy link

@rabrad rabrad commented Aug 20, 2019

For review

Copy link
Contributor

@wilgert wilgert left a comment

Choose a reason for hiding this comment

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

You did very well. Don't let my negative comment discourage you. I say those to help you grow!

const ul = document.createElement('ul');

// Use "for in" loop with objects
for (const book in myBooks) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The instructions asked you to loop over the book titles in the myBookTitles array. And use each title to access the correct data from the myBooks object.
This is because the order of keys in an object is not guaranteed, while the order of items in an array is guaranteed. See: https://stackoverflow.com/questions/5525795/does-javascript-guarantee-object-property-order
So in the way that you build it the page could show the books in a different order on different computers.

Please change the booksInfo function so it loops over the myBookTitles array.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your feedback. I'm working on it

};

function bookCovers() {
for (const book in myBooks) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function looks really good. Great job!

@@ -1 +1,52 @@
/* add your styling here */ No newline at end of file
/* add your styling here */
* {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice design! And good job for making it responsive!

for (let i = 0; i < bookTitles.length; i++) {
const createHeading = document.createElement('h3');
document.getElementById(bookTitles[i]).appendChild(createHeading);
const bookInfo = Object.keys(myBooks);
Copy link
Contributor

Choose a reason for hiding this comment

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

This bookInfo constant is not needed. You can do bookTitles[i] to get the object that holds the info of the book, just like you did on the line above.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your helpful feedback.
I have modified the code, and updated some links to improve the quality of the cover images.

  • Please ignore the debugging sample files.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants