-
Notifications
You must be signed in to change notification settings - Fork 34
Tom Bastian News Site #32
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
| <button class="normButton">Sport</button> | ||
| <button class="normButton">Entertainment</button> | ||
| </div> | ||
| <div class "searchsection"> |
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.
missing =
| <div class "searchsection"> | ||
| <form class="form"> | ||
| <div class="text-input"> | ||
| <textarea class = "textarea" rows="1" cols="10"> |
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.
text input would work better for single line search input
| let fetchPromise = fetch('https://newsapi.org/v2/top-headlines?country=gb&apiKey=56887af3601845008f0dc6fa7f8b8810') //fetchs default page -news in UK | ||
|
|
||
|
|
||
| const Buttons = document.querySelectorAll(".normButton") |
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.
name variables and functions using lower case for first letter.
|
|
||
| let textInBox = "" // variable to hold the value input to the text area | ||
| textArea.addEventListener("input", function(event) { | ||
| textInBox = event.target.value |
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.
It might be easier to grab the text input content, once submit is detected. It will save writing an additional event handler and save using a global variable
|
|
||
|
|
||
|
|
||
| /*function createCharacter(character){ |
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.
Commented out code can be removed
|
Writing code in functions is always good. Keeps code clean, concise and re-usable. It looks like the infinite scroll issue is due to api call failing when query is not defined. If you try load on scroll without typing in a query, the api call fails |
|
It would be good to see a README that explains what the code does so people reviewing your github account in future will know what you have done |
|
To spot issues like scrolling in future take a look in dev tools for any errors. they might contain clues about what is breaking |
Do you know why this broke in demo? I can't replicate and fix :(. Could it be to do with the "isLoading" variable and the API being slow?
Secondly, any advice on how the code is written and making it better. I noticed Phil writes all his code in functions, which I kind of liked.
Thanks,
Tom