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

Make Pagination Component currentPage reset conditional when number of pages changes #3716

Closed
jaylittle opened this issue Jul 19, 2019 · 11 comments · Fixed by #3990 · May be fixed by thislooksfun/earthdawn#37

Comments

@jaylittle
Copy link

commented Jul 19, 2019

Regarding the Pagination component: Would it be possible to make the currentPage reset in the numberOfPages watch conditional using the following logic?

this.currentPage = (newVal >= this.currentPage ? this.currentPage : 1);

The reason being is that I have a situation in which I am using the pagination controls but I have no way of knowing what the total number of rows is in advance. So as the user goes to the next page, I am increasing the total row count based on whether or not the previous page returned a full page of results or not. So this means that when the page size is 25, total rows is set to 26 when we render page 1. When the user requests page 2, we acquire the data and set total rows to 51 if page 2 returned a full 25 rows (incrementing the total number of available pages) or set it to 50 if it did not (which keeps the total number of pages at 2). In the first situation, when data for page 3 is available the pagination control is now changing the page number back to 1 and I have no way to prevent this within code.

TLDR: I basically don't want currentPage to reset back to 1 unless the currentPage value has actually been invalidated by the change in the total number of pages (e.g. currentPage > newTotalNumberOfPages). Alternatively if we could just add a setting on the component that disables page number resetting, that would also work.

Thanks.

@tmorehouse

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

We would also need to check if per-page (for b-pagination) has changed as well, to account for page size changes, and try and keep the user on the new page which had the items they were looking at.

Since the underlying logic is shared with both b-pagination and b-pagination-nav, we just need to be carefull not to make it introduce bugs in either pagination component.

@tmorehouse tmorehouse self-assigned this Jul 19, 2019

@jaylittle

This comment has been minimized.

Copy link
Author

commented Jul 19, 2019

That makes sense. Would it be possible to just add a component property that allows us to disable automatic current page number updates entirely? That would effectively solve my problem.

@tmorehouse

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

That might require a bit of additional logic as it could end up (if not used correctly) an unusable user interface (which it why the page number reset was added in the first place)

@jaylittle

This comment has been minimized.

Copy link
Author

commented Jul 19, 2019

That's fair. It's worth emphasizing that in this case, page size hasn't changed. Only the total number of rows has changed.

@tmorehouse

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

But it is possible for total pages to change because of per-page (cascade effect)

@jaylittle

This comment has been minimized.

Copy link
Author

commented Jul 19, 2019

Perhaps the solution is to simply have two separate watchers instead of a single watcher on numberOfPages then. One for perPage and one for totalRows. perPage watcher can modify currentPage in whatever way it feels is appropriate whereas totalRows watcher maybe doesn't?

@jaylittle

This comment has been minimized.

Copy link
Author

commented Jul 19, 2019

Actually maybe we just need a new watcher for perPage that handles the update for currentPage:

watch: {
  numberOfPages(newVal) {
    if (newVal === this.localNumPages) {
      /* istanbul ignore next */
      return
    }
    this.localNumPages = newVal
  },
  perPage(newVal, oldval) {
    //TODO Handle preservation of current user view
    this.currentPage = 1
  }
},

EDITED: That won't work either, lets try this instead:

watch: {
  numberOfPages(newVal) {
    if (newVal === this.localNumPages) {
      /* istanbul ignore next */
      return
    }
    this.localNumPages = newVal
    if (this.changeSource && this.changeSource === 'totalRows') {
      this.currentPage = (this.currentPage <== newVal ? this.currentPage : 1)
    } else {
      /* TODO Handle preservation of current user view */
      this.currentPage = 1
    }
  },
  perPage(newVal) {
    this.changeSource = 'perPage'
  },
  totalRows(newVal) {
    this.changeSource = 'totalRows'
  },
},

Then be sure to add an initialization for this.changeSource to created(), right?

@tmorehouse

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

It could be something like that... but will require a bit of testing for various scenarios for both b-pagination and b-pagination-nav (which perPage is always 1, or doesn't exist)

tmorehouse added a commit that referenced this issue Jul 21, 2019

@tmorehouse tmorehouse reopened this Jul 21, 2019

@jaylittle

This comment has been minimized.

Copy link
Author

commented Jul 22, 2019

I guess one way or another that commit is destined to close this issue, eh?

@tmorehouse

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

LOL... yeah... darned github automation...

@tmorehouse tmorehouse reopened this Jul 22, 2019

@tmorehouse tmorehouse added this to To Do in 2.0.0 Stable Jul 22, 2019

@tmorehouse tmorehouse added this to To do in 2.1.0 Aug 30, 2019

@tmorehouse tmorehouse removed this from To do in 2.1.0 Aug 31, 2019

2.0.0 Stable automation moved this from To Do to Completed Aug 31, 2019

tmorehouse added a commit that referenced this issue Aug 31, 2019
@tmorehouse

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

BootstrapVue v2.0.0 stable has been released. See https://bootstrap-vue.js.org/docs/misc/changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.