facn2 / knafeh Goto Github PK

Background of the app seems quite boring to me. Maybe add a nice image? possibly using another api call?

images and titles on app for israeli stories from guardian

flag is no longer a background image so change class name to something more appropriate 🇬🇧 🇰🇷 🇭🇰 🇮🇱

please add test.js file.

https://github.com/facn2/knafeh/blob/master/scripts/api.js#L6

what happens if the request does not get a 200 response?

You've done loads of nice abstraction, one thing I would say is that you could have turned the bit where you update the flag picture into its own function, like you did with your update story function.

https://github.com/facn2/knafeh/blob/master/scripts/api.js#L34

it's not available for me to scroll for more news on the same page or to move to other pages, it would be nice if we had the chance to look for more news.

Perhaps some lines / functions could have been commented for instance:

storySummary.innerText = result.fields.bodyText.split('. ').slice(0, 4).join('. ');

https://github.com/facn2/knafeh/blob/master/scripts/updateContent.js#L3-L13

For each reload, you make changes to the page (a relatively expensive action) lots of times.

It is better to create the elements then append them to the page in 1 action.

No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'null' is therefore not allowed access.

lots of code in api.js not very readable
split into separate functions/files

create overall file structure for the app

start styling the app
mobile first design/responsive

The news need to be updated when a user choose another country on options

Your API's should bring the last updated News, not old ones, there are many old News and as a visitor to Israel I want to be able to read the latest Israeli news

There's two files with dom manipulation in them - updateContent and createTemplate. I can see the reason for splitting them across files, but seeing as they are all dom manipulation, I would think about having them in the same file, or giving them both similar names - createDomElements, updateDomElements

Istanbul ? 🍁

Here in the test file

Fetch is normally a function that makes an API request, so this naming was a little confusing - maybe "makeFlagURL" or something

Only using two calls so maybe not 100% necessary but you could have done a function like we did in the workshop. Both calls would pass their results to an array which would then be used in one final callback, which would render both the stories and the flags at the same time.

Hide it

https://gist.github.com/derzorngottes/3b57edc1f996dddcab25

I guess the font family is not working.

https://github.com/facn2/knafeh/blob/master/scripts/api.js#L8

maybe adding a flag next to option bar would be nice!

@sajedazoabi pointed out that if you click on a news story and then return back to kanefh news page the displays the news from the same country but labels it as a different country. See Screenshot.

https://github.com/facn2/knafeh/blob/master/scripts/global.js#L3

Like, it might be if you had two news URL's and this was the second one, but even then you could be more specific.

However here, it's not even a second URL it's another part of the URL. Better practice might be to have baseURL, APIKeyURL, showFieldsURL as three parts of the URL

Can you guys put a link to your website in readme section?

Single quotes here: https://github.com/facn2/knafeh/blob/master/index.html#L11
and double here???: https://github.com/facn2/knafeh/blob/master/index.html#L13

A width and height should be specified for all images in order to speed up page display. check Audits.

You can remove lines 52-54 from the css file
https://github.com/facn2/knafeh/blob/master/style/style.css#L53

https://api.monosnap.com/rpc/file/download?id=Oq2fvib66tzCMNdBfNd3QVIAiDHRLJ

I thought newest but then UK election result is top for UK - so I guess relevance or something? Was this a conscious decision or the default?

The flag looks pretty cool but it is not positioned inline with the option menu.

On my screen the option bar slightly bigger and pushed further up than the flag.

please add a drop-down menu so that a client can navigate to news from their home country

include alt text for images to improve accessibility

create new div so flag doesn't append to the dropdown menu

https://github.com/facn2/knafeh/blob/master/index.html#L9

in iPhone 5 you can't see the descriptions about the article.

I saw you doing them, they would be a welcome addition...

please add js file to make API call

README.md is missing

e.g. https://github.com/facn2/knafeh/blob/master/scripts/createTemplate.js#L11

article paragraphs should appear under heading on desktop

Please add the URL to news title

index.html file is missing in the repo

With scripts to run things would be nice

please add css file including media queries for various devices