Responsive-news-reader #31
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| <div id="newsfeed"> | ||
|
|
||
| </div> | ||
| <button onclick="topFunction()" id="myBtn" title="Go to top">Go to Top</button> |
Contributor
There was a problem hiding this comment.
It would be better to keep event handler setting in JS rather than apply inline
| }); | ||
|
|
||
| // Tried adding an event listener to 'More news from this source button', but it does not seem to work. | ||
| document.querySelectorAll(".news").addEventListener("click", e => { |
Contributor
There was a problem hiding this comment.
querySelectorAll returns a NodeList and we can't set an event handler on it. It would have to be done on each element in list individually
|
|
||
| //Two functions are executed on scroll - sticky header and 'go to top' button | ||
| window.onscroll = function() { | ||
| myFunction(), scrollFunction(); |
Contributor
There was a problem hiding this comment.
the function calls should be separated by ; rather than ,
| var header = document.querySelector(".header"); | ||
| var sticky = header.offsetTop; | ||
| // Add the sticky class to the header when you reach its scroll position. Remove "sticky" when you leave the scroll position | ||
| function myFunction() { |
Contributor
There was a problem hiding this comment.
function could have a more descriptive name
| <div id="newsfeed"> | ||
|
|
||
| </div> | ||
| <button onclick="topFunction()" id="myBtn" title="Go to top">Go to Top</button> |
Contributor
There was a problem hiding this comment.
myBtn does not tell much about what the button does. Something more descriptive would work better
Contributor
|
IT would be great to see an updated README that explains what the project does and any stretch goals you have created |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unfortunately I still have not managed the 'More news button to work' :(