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

Luke Sikuade News Reader #28

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

Conversation

ljsikuade
Copy link

No description provided.

README.md Outdated

- [ ] Display images for tablet and desktop screens only
Simply download and unpack the .zip file onto your machine, then open up the index.html.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a zip file?

"uri" should be undefined the first time, but not subsequent times
for purposes of pagination.
*/
function grabData(uri) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than using if/else you can use default parameter values which will replace the parameter value when it is undefined. For example
function grabData(uri = url){
That way if uri is undefined, the url value will be applied instead

//As they pass in, create list elements from them.
//Call populateNewsFeed on each one.
function createListElement(articleObject) {
/*CREATE THE LIST ITEM WITH ATTRIBUTES AND ARTICLE CONTENT */
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be easier to generate using HTML string templates?

}

//PROGRAM START
document.addEventListener("DOMContentLoaded", grabData());
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@dmitrigrabov
Copy link
Contributor

Good work

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.

2 participants