-
Notifications
You must be signed in to change notification settings - Fork 34
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
Kate news reader #16
base: master
Are you sure you want to change the base?
Kate news reader #16
Conversation
newsReader.js
Outdated
const keyword=document.querySelector(".keyword"); | ||
let p=1; | ||
|
||
let urlBase = 'https://newsapi.org/v2/' |
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.
variables which will not change in future should be declared using const
newsReader.js
Outdated
return response.json() | ||
}) | ||
.then(response => { | ||
console.log(response); |
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 would be good to extract the display logic into own function to keep the fetch code concise and make the display code re-usable if needed in future.
newsReader.js
Outdated
articleList.appendChild(newsDetail); | ||
i++; | ||
} | ||
return; |
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.
return is not needed
newsReader.js
Outdated
.catch(error => console.log(error)); | ||
|
||
|
||
nextPageButton.onclick=function(event) { |
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 would be better to set the event listener using addEventListener
newsReader.js
Outdated
return response.json() | ||
}) | ||
.then(response => { | ||
console.log(response); |
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.
I would prefer to see display logic in own function
newsReader.js
Outdated
.then(response => { | ||
console.log(response); | ||
|
||
i=1; |
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 can get an index number from forEach
. It is the second parameter passed to it. For example,
response.articles.forEach(function(item, index){ })
i++; | ||
document.documentElement.scrollTop = 0; | ||
} | ||
return; |
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.
return is not needed
return response.json() | ||
}) | ||
.then(response => { | ||
console.log(response); |
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.
Same comments as before regarding use of i
and wrapping display logic in function
It would be good to see a README update which explains what the project does. Very nice work otherwise |
No description provided.