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

add pagination properly according to json-api #2

Merged
merged 1 commit into from
Aug 21, 2015

Conversation

bacarini
Copy link
Owner

Pagination links is not as json-api says it should be. So, this PR is not only to fix that, but also to make it works with kaminairi as well. ❤️ 😄

@bacarini
Copy link
Owner Author

@akaKuruma and @rafaellima what is your thoughts about this PR?


unless collection.current_page == FIRST_PAGE
pages[:first] = FIRST_PAGE
pages[:prev] = collection.current_page - FIRST_PAGE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but I believe that pages[:prev] should be current - 1, not current - FIRST_PAGE. This considering that the FIRST_PAGE value change to 0, the value of the previous page remains current - 1.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, FIRST_PAGE is 1, You might had misunderstood because of the name.

But I do agree with you that is quite difficult to read, so I've change the name to ONE_PAGE. What do you think?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ONE_PAGE 👌

@bacarini bacarini force-pushed the feature/pagination branch from 01b49da to 10eeb5c Compare August 21, 2015 15:07
bacarini added a commit that referenced this pull request Aug 21, 2015
add pagination properly according to json-api
@bacarini bacarini merged commit 89415e1 into master Aug 21, 2015
@rafaellima
Copy link

@bacarini kudos 👍

@bacarini bacarini deleted the feature/pagination branch August 21, 2015 18:39
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.

3 participants