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

feat: add pagination support #84

Closed
wants to merge 1 commit into from

Conversation

maciek134
Copy link

Added pagination support, resolves #68 (and more, since it's supported in all feed types).

@coveralls
Copy link

Coverage Status

Coverage increased (+3.4%) to 99.029% when pulling 1009a97 on maciek134:feat/pagination into c4666db on jpmonette:master.

@maciek134
Copy link
Author

@jpmonette actually if you want I could add proper pagination support - so the library would paginate automatically and you could get specific pages using a method like .getPage(n: number)

@jpmonette
Copy link
Owner

@maciek134 I'm not sure how this would be maintainable in the current state - seems like a lot of maintenance for a developer to add pagination - the developer would have to create many instances of Feed and manually set next / prev, no?

Please, open an issue instead with what you want to achieve and suggested designs decisions as I'm not sure this is the best approach. I'm sure this could be valuable for the library, but in a different shape.

Thanks for the time spent here.

@jpmonette jpmonette closed this Aug 11, 2019
@vedmant
Copy link

vedmant commented Aug 19, 2022

So this was rejected? Pity, only because it has no pagination I can't use this library

@maciek134
Copy link
Author

So this was rejected? Pity, only because it has no pagination I can't use this library

And with good reasoning. I missed the reply though, otherwise I would have tried to fix the PR. I had an idea for a proper solution back then (that doesn't require so much manual work) but I needed this quickly and just shared my "fix". I'll try to revisit when I have time.

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.

atom:link next
4 participants