Skip to content

Paginator Order Context#225

Merged
lgebhardt merged 1 commit intomasterfrom
214
May 30, 2015
Merged

Paginator Order Context#225
lgebhardt merged 1 commit intomasterfrom
214

Conversation

@lgebhardt
Copy link
Copy Markdown
Contributor

This updates #214 with the new changes in master.

To build paginators that rely on sort order (e.g. cursor pagination) the
sort order requested must be known when applying pagination. This adds
an additional argument to Paginator#apply to supply the current sort
order and hooks it in through the resource finders.

@lgebhardt
Copy link
Copy Markdown
Contributor Author

@john-griffin I updated your PR (#214) with the changes in the latest master. Could you please take a look at the changes and confirm they will work with your cursor based approach?

Also, if you wouldn't mind I'd love to get cursor based pagination into the project. If you could create a PR with that it would be appreciated very much.

@john-griffin
Copy link
Copy Markdown
Contributor

@lgebhardt thanks for updating the PR - this gets my cursor pagination tests passing. I'll for sure get a PR up for it. It's a little specific to our app right now so I might spend sometime making it more generic. 👍

@lgebhardt
Copy link
Copy Markdown
Contributor Author

@john-griffin Thanks for confirming. I'll merge once Travis finishes.

To build paginators that rely on sort order (e.g. cursor pagination) the
sort order requested must be known when applying pagination. This adds
an additional argument to Paginator#apply to supply the current sort 
order and hooks it in through the resource finders.
@lgebhardt
Copy link
Copy Markdown
Contributor Author

@john-griffin I had to rebase again due to another PR. If you have a minute could you confirm that the code still works?

@john-griffin
Copy link
Copy Markdown
Contributor

@lgebhardt yep still good here

lgebhardt added a commit that referenced this pull request May 30, 2015
Paginator Order Context
@lgebhardt lgebhardt merged commit 7335d83 into master May 30, 2015
@lgebhardt lgebhardt deleted the 214 branch May 30, 2015 01:14
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.

2 participants