-
Notifications
You must be signed in to change notification settings - Fork 0
Working #1
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: master
Are you sure you want to change the base?
Conversation
| var content = document.getElementById("content"); | ||
| var search = document.getElementById("search"); | ||
|
|
||
| search.addEventListener("click", () => { |
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.
Use an abstraction for the repeated code
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.
Didn't think of that before. Will do that now. Thank you
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.
Done
… catch empty search querys
js/index.js
Outdated
| var request = new XMLHttpRequest(); | ||
| request.open('GET', url); | ||
| request.onload = () => { | ||
| if (request.status >= 200 && request.status < 400) { |
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 success status code will always be 200. So if (request.status === 200) should be a sufficient check.
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.
done. Thanks
js/index.js
Outdated
| var books = data.items; | ||
| var eligible = []; | ||
| for (x = 0; x < books.length; x++) { | ||
| if (books[x].volumeInfo.imageLinks != null && books[x].volumeInfo.authors != null && books[x].volumeInfo.industryIdentifiers != null) { |
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.
Multiple if/else usually make the code difficult to read. Think of refactoring it or maybe see if you can use a switch
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.
it's a single if-else statement but has multiple conditions, and I think a switch statement will be longer. please advise appropriately
js/index.js
Outdated
|
|
||
| window.onload = () => { | ||
| url = "https://www.googleapis.com/books/v1/volumes?q=pride+prejudice&maxResults=40" | ||
| url = "https://www.googleapis.com/books/v1/volumes?q=pride+prejudice&download=epub&maxResults=40" |
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.
You are repeating this URL. Declare it in the global scope and reuse it
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.
Done. had to use a function however
Merging working branch with master