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

Fix hasPrevPage behaviour with limit > offset > 0 #144

Merged
merged 2 commits into from
Jan 6, 2022
Merged

Fix hasPrevPage behaviour with limit > offset > 0 #144

merged 2 commits into from
Jan 6, 2022

Conversation

chdanielmueller
Copy link
Contributor

continue on #133

@aravindnc
Copy link
Owner

Thanks but again my question arises, why we are setting prev page as 1. Since its already loading 1st page.
This could break somebody's front end implementation - which will show 0th page since we are enabling prevPage=true for 1st page.

Waiting for your thoughts.

@chdanielmueller
Copy link
Contributor Author

The thing is since we set offset: 1 we are "in the middle" of page 1.
Think of it as page 1.5.

Which means we can go to the start of page 1 or continue to page 2

@aravindnc
Copy link
Owner

But, still we cannot go back to page 1, since its passing offset which loads the same page again. Moreover the frontend pagination can go haywire if current page is 1 and previous page is 1. Correct me if I'm wrong.

@chdanielmueller
Copy link
Contributor Author

Yes, that happened to me when you changed the behavior in 277d3f3
I rely on the existing behaviour because if a user changes the number of elements per page (limit e.g. 5 > 10) and was already on page 2 of the previous query (e.g. offset 5) he needs to be able to go back to the complete page 1 starting at record 0.

Please check src/index.js line 210 - 212.
My change is to revert this change which was the old behavior.

Your change was made because of #119 where the issue was to be on page 1 with an offset of 1 showed a previous page.
Which would be correct if you ask me because there is data that is not shown on that page.
The real solution to his issue would have been to point out setting offset to 0 or undefined.
That way he would not loose the first element on the first page.

He later told that he was not loosing the first element with offset 1 (which would mean offset does not work as expected): #133 (comment)
That's why I added the test first page with page and limit, limit > doc.length, offset by one
This test proofs that offset works as expected and his comment can not be valid. (max elements: 100, limit: 200 -> page: 1, offset: 1 -> returns 99 elements)

In my opinion it can be a problem for someone new to this library which sets offset to 1 and not getting feedback that there is more data to be gatherd (prevPage).

I think now it is up to you to decide which behavior you would like to support.

@aravindnc
Copy link
Owner

Thanks @chdanielmueller for the prompt response. Let me go through this and will get back.

@aravindnc aravindnc merged commit 0cc238b into aravindnc:master Jan 6, 2022
@aravindnc
Copy link
Owner

Make sense. Package has been update with this change.
Thanks @chdanielmueller, really appreciate your contribution and patience.

@chdanielmueller
Copy link
Contributor Author

Thanks @aravindnc for contributing your time and skill for the open source community 👍

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.

None yet

2 participants