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.
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.
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; |
newsReader.js
Outdated
| .catch(error => console.log(error)); | ||
|
|
||
|
|
||
| nextPageButton.onclick=function(event) { |
There was a problem hiding this comment.
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.
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.
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; |
| return response.json() | ||
| }) | ||
| .then(response => { | ||
| console.log(response); |
There was a problem hiding this comment.
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 |
2 participants
No description provided.