Skip to content
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

Responsive News Reader #18

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

DavidGridley
Copy link

No description provided.

})
.then(body => {
// pullNewsObject(body);
createNewsContent(body);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

//Create .innerHTML of each news article div.

function articleTemplate (article) {
let authorVar = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Code would be slightly cleaner if declaration and assignment was combined into single step. That would also allow you to use const instead of let

function createSideBarContent(news) {
news.articles.forEach(item => {
if (item.content !== null && item.source.name !== "Bloomberg") {
const sideBar = document.querySelector(".news_sidebar");
Copy link
Contributor

Choose a reason for hiding this comment

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

featureBox and sideBar lookup can be taken outside the forEach. Will save you having to run querySelector up on each loop iteration.



//Adds more items to the sidebar as the page number for main article increases
function updateSideBarUrl(number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good, but I would suggest not putting the fetch call in this function. Use this function just to generate the URL and return it. Then the returned value can be used to call generateSideBar. This will reduce the amount of responsibility this function has and make it easier to test later as we will discuss later this week.

@dmitrigrabov
Copy link
Contributor

It would be great to see a README that explains what the project does

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants