-
Notifications
You must be signed in to change notification settings - Fork 10
626 paginate endless scrolling on saves #628
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
Conversation
✅ Deploy Preview for voluble-nougat-015dd1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
- Works similarly to the one used in FindArticles, by updating the list with extra articles until we do not get anything from the API. - Each call takes about 1.4s - 1.6s in my development environment (to fetch 20 articles). Before, this user had about 76 articles taking 6.3s to fetch all.
c9b8881
to
25df96f
Compare
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.
looks good.
small question about extracting common functionality between two components. if you say it's not worth it, we merge.
src/articles/OwnArticles.js
Outdated
setIsWaitingForNewArticles(false); | ||
} | ||
|
||
function handleScroll() { |
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.
there's a bit of duplication now between OwnArticles
and FindArticles
. I think we can live with it, but at the same time, if there was an easy way to extract a useArticlePagination
hook from the two it might be good to do. But if it's too complicated, then it's fine.
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.
Indeed, I had thought about this - but I was a bit unsure about how to do it in a way that would allow for different behaviours depending on which component we are using the pagination from.
I have just pushed what I ended up coding - let me know what you think
- Renamed variables to match the name suggested by Mircea.
- Remove hardcoded setTitle
…tion. piece of cake :)
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.
cool stuff @tfnribeiro !
Requires the PR in the API: zeeguu/api#292
Note: I removed the isMounted check as it is an anti-pattern (we want to see the warning in case it's an issue). We should try to follow a pattern similar to the one shown here: https://legacy.reactjs.org/blog/2015/12/16/ismounted-antipattern.html