-
Notifications
You must be signed in to change notification settings - Fork 0
week -3 task to be reviewed #3
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
Week-3/scripts/Render.js
Outdated
| } | ||
| } | ||
| function setLeftContent(fullContainer,container){ | ||
| let url = `https://newsapi.org/v2/top-headlines?country=in&apiKey=ac1fd7a9fc1342abb913e554a9525d85&pageSize=20`; |
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.
move url to constant file
Week-3/scripts/constants.js
Outdated
| @@ -0,0 +1,17 @@ | |||
| const defaultContent='Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.'; | |||
| const defaultDescription='created at 21 July 2019""created at 21 July 2019'; | |||
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.
can use single const declaration
Week-3/scripts/Utility.js
Outdated
| window.localStorage.setItem(emailid, JSON.stringify(emailid)); | ||
| alert(emailSubscribedMessage); | ||
| } | ||
| else{ |
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.
generally if else block is indented as below example
if(condition) {
//code
} else if(condition) {
//code
} else {
//code
}
Week-3/scripts/Utility.js
Outdated
| popup.appendChild(descriptionPopup); | ||
| fullContainer.style.opacity = '0.3'; | ||
| } | ||
| else if(clickEvent.srcElement.id === 'closePopup'){ |
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.
generally if else block is indented as below example
if(condition) {
//code
} else if(condition) {
//code
} else {
//code
}
Week-3/scripts/Update_View.js
Outdated
| btn.heading=contentHead; | ||
| } | ||
| else{ | ||
| btn.data=data.articles[index].description; |
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.
generally if else block is indented as below example
if(condition) {
//code
} else if(condition) {
//code
} else {
//code
}
Week-3/scripts/Render.js
Outdated
| }) | ||
| .catch(function(error) { | ||
| }); | ||
| fullContainer.appendChild(container); |
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.
shouldnt be this line inside the success?
Week-3/scripts/Render.js
Outdated
| .catch(function(error) { | ||
| }); | ||
| fullContainer.appendChild(container); | ||
| } |
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.
for this assignment trainer asked to keep in header only
Week-3/scripts/Render.js
Outdated
| @@ -0,0 +1,41 @@ | |||
| function setRightContent(article_data,fullContainer,container){ | |||
| let rightContainer = document.createElement('div'), | |||
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.
fix indentation for this function
Week-3/scripts/Render.js
Outdated
| } | ||
| } | ||
| function setLeftContent(fullContainer,container){ | ||
| let url = `https://newsapi.org/v2/top-headlines?country=in&apiKey=ac1fd7a9fc1342abb913e554a9525d85&pageSize=20`; |
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.
fix indentation for this function
Week-3/scripts/Update_View.js
Outdated
| function changeContent(val,fullContainer,container){ | ||
| let data=fullContainer.data; | ||
| let similar=0; | ||
| container.innerHTML = ''; |
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.
fix indentation for this function
Week-3/scripts/Render.js
Outdated
| .catch(function(error) { | ||
| }); | ||
| fullContainer.appendChild(container); | ||
| } |
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.
for this assignment trainer asked to keep in header only
| @@ -0,0 +1,10 @@ | |||
| function setLeftContent(fullContainer,container,newsUrl){ | |||
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.
fix indentation for the file
| @@ -0,0 +1,78 @@ | |||
| function changeContent(container,fullContainer){ | |||
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.
fix indentation for the file
Week-3/scripts/Utility.js
Outdated
| @@ -0,0 +1,40 @@ | |||
| function clicked(clickEvent,fullContainer,popup){ | |||
| return clickEvent => { | |||
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.
fix indentation for the file
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
|
looks good |
No description provided.