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 option to reset pagination when last page < current page #3987

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

antman3351
Copy link
Contributor

Hi Oli,

I had a go at implementing the feature request #3962.

I added the option paginationLastPageErrorResetTo that expects a value of first or last with the default of last.

I'm not convinced by the return from my change (/src/js/modules/Page/Page.js@857).
Also would it be best to the the function finish loading ( dispatching the 'pageLoaded' event ) then reload the page ?

Let me know if I need to change anything :)

Cheers
Antonio

@olifolkerd
Copy link
Owner

Hey,

Thanks for the pr. Sorry for the delayed reply, I have been focusing on the website the last couple of weeks

I have a couple of points of feedback.

When you are changing the page no, I would recommend calling the modules internal setPage function instead of updating the variable yourself. That will trigger all the events and request that you need instead of replicating functionality.

You can also pass the word first or last directly to that function and it will take care of the rest.

The default behaviour should probably be to do nothing as it currently does, to allow for people to handle it differently. You could also allow the user to not just pass a string but to pass a callback that could then return the page incase they have a more complex usage case

As for the option name that is a bit on the verbose side. Maybe something like paginationOutOfRange or paginationMaxExceeded

Cheers

Oli :)

Renamed option paginationOutOfRange
option paginationOutOfRange can be a function that returns the page
internally call this.setPage
@antman3351
Copy link
Contributor Author

Hey Oli,

I made the changes as you suggested.

Just two things:

Is it ok that the DataLoader issues a warning Data Load Response Blocked - An active data load request was blocked by an attempt to change table data while the request was being made. ?

Also is the call to the user supplied function ok paginationOutOfRange.call(this, this.page, this.max) ?

@cgountanis
Copy link

cgountanis commented Nov 10, 2023

@antman3351 Sorry if this was covered but has this been implemented in the latest releases? We are running into same issue where last page is higher than dataset on revisits using built-in persistence enabled. Simple setting?

e.g. paginationOutOfRange: any documentation?

Edit: Even just simply no settings but error handle and set to page 1 if persistent page is > available. That would be even better!

Copy link

@cgountanis cgountanis left a comment

Choose a reason for hiding this comment

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

Would be cool if this was an option as of right now we get errors when dataset gets smaller and persistent page is > then available. Even just simply no settings but error handle and set to page 1 if persistent page is > available. That would be even better!

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

3 participants