feat(pagination): Add custom pagination with variable page sizes #5174

chatcher · 2016-02-28T16:36:51Z

Use Case:
Client requests page breaks by (e.g.) Date column. This can currently be
accomplished by external pagination, but the 'page size' dropdown
becomes irrelevant, and the page-count on the right can no longer
correctly determine the correct row range.

The pagination template has been modified to hide the page-size dropdown
when custom pagination is enabled, and to use new API methods for the
row range.

A new tutorial page has been added as lesson 406-Custom-Pagination
which demonstrates two usages: pre-sorted data, with pre-calculated pages
sizes; and unsorted data, with external pagination.

Use Case: Client requests page breaks by Date column. This can currently be accomplished by external pagination, but the 'page size' dropdown becomes irrelevant, and the page-count on the right can no longer correctly determine the correct row range. The pagination template has been modified to hide the page-size dropdown then custom pagination is enabled, and to use new API methods for the row range.

chatcher · 2016-02-28T16:47:31Z

~~hmm, this passes locally...~~
Tests have been fixed.
New tests have been added.

instead of calling reduce another time.

AgDude · 2016-07-30T11:28:25Z

This looks like a very specific use case. I think I would prefer to see this in an independent plugin (or all of the pagination UI in a plugin for that matter).

mportuga · 2016-09-20T20:52:03Z

@chatcher Can you fix unit tests?

page index and page number were confused.

mportuga

I made some comments on your code now. I had some questions, one request and one suggestion. Thank you for fixing the unit tests earlier however.

mportuga · 2016-09-25T17:30:09Z

src/features/pagination/js/pagination.js

@@ -146,12 +176,14 @@
            var visibleRows = renderableRows.filter(function (row) { return row.visible; });
            grid.options.totalItems = visibleRows.length;

-            var firstRow = (currentPage - 1) * pageSize;
+            var firstRow = publicApi.methods.pagination.getFirstRowIndex();
+            var lastRow  = publicApi.methods.pagination.getLastRowIndex();


These definitions can be comma separated.

like so?

var firstRow = blahBlah(), lastRow = blahBlah();

I don't see that pattern used elsewhere. Can you clarify what you mean?

that is what I meant. But I suppose you are right and we are not using this anywhere else in this project right now. It is my personal preference. No need to change it, in that case.

mportuga · 2016-09-25T17:31:02Z

src/features/pagination/js/pagination.js

            if (firstRow > visibleRows.length) {
              currentPage = grid.options.paginationCurrentPage = 1;
              firstRow = (currentPage - 1) * pageSize;
            }
-            return visibleRows.slice(firstRow, firstRow + pageSize);
+            return visibleRows.slice(firstRow, lastRow);


Your function for getLastRowIndex doesn't appear to match the firstRow + pageSize calculation that I see here. Am I missing something?

firstRow is (currentPage - 1) * pageSize
Previously, lastRow was firstRow + pageSize, which is algebraically

lR = fR + pS | fR = (cP - 1) * pS = (cP-1) * pS + pS | distribute pS over (cP-1) = cP*pS - pS + pS | cancel addition = cP*pS

With the change, lastRow is currentPage * pageSize (or totalItems), which simply eliminates the math in the middle.

Thanks for the lesson in algebra right now. I should have caught that.

mportuga · 2016-09-25T17:33:23Z

src/features/pagination/js/pagination.js

+                  if (grid.options.useCustomPagination) {
+                    return publicApi.methods.pagination.getFirstRowIndex() + grid.options.paginationPageSizes[grid.options.paginationCurrentPage - 1];
+                  }
+                  return Math.min(grid.options.paginationCurrentPage * grid.options.paginationPageSize, grid.options.totalItems);


Why is Math.min being used here? That doesn't appear to be there in the original function for when useCustomPagination is false. Shouldn't this be the following?
publicApi.methods.pagination.getFirstRowIndex() + grid.options.paginationPageSize

This was taken from $scope.showingHigh (line 372), which is replaced by 1+getLastRowIndex().

mportuga · 2016-09-25T17:34:34Z

src/features/pagination/js/pagination.js

@@ -368,8 +410,6 @@
          $scope.$on('$destroy', dataChangeDereg);

          var setShowing = function () {


It looks like this is an empty function now. Was that on purpose? If so, what is the intention here?

That was in the original code; I could not find a usage or reassignment of it, but I didn't want to touch it in case I was wrong.

set showing is called in lines 415 and 432. One of those usages also creates a watch, I would argue that if this function can definitely stay empty that you should remove this function, the watch on line 415 and the usage on line 432. Also, the call to destroy said watch on line 438

mportuga · 2016-09-25T17:39:33Z

src/features/pagination/test/pagination.spec.js

@@ -64,6 +64,8 @@ describe('ui.grid.pagination uiGridPaginationService', function () {
  describe('initialisation', function () {
    it('registers the API and methods', function () {
      expect(gridApi.pagination.getPage).toEqual(jasmine.any(Function));
+      expect(gridApi.pagination.getFirstRowIndex).toEqual(jasmine.any(Function));


You should add more unit tests for these two functions than simply checking if they exist and are functions. Would you mind adding tests to ensure they return what is expected?

I've added tests for custom pagination.

Rather than testing getFirstRowIndex and getLastRowIndex directly, we test the resulting pages.

The previous tests covered one branch inside the two methods, and the tests I've added cover the other branch.

mportuga · 2016-09-25T17:40:18Z

src/features/pagination/js/pagination.js

@@ -368,8 +410,6 @@
          $scope.$on('$destroy', dataChangeDereg);

          var setShowing = function () {
-            $scope.showingLow = ((options.paginationCurrentPage - 1) * options.paginationPageSize) + 1;
-            $scope.showingHigh = Math.min(options.paginationCurrentPage * options.paginationPageSize, options.totalItems);


Are showingHight and last page index really the same thing? If so, how did you come to that conclusion?

The usage of showingLow and showingHigh was in the template to show the row range.
Instead of calculating the range here, I instead call the new methods from the template and add one there.

Since custom pagination allows for arbitrary pages of rows, we pick some feature of the existing data to paginate by. I've chosen the approximate number of pencil strokes to write the letters on column 2 (with a line through 'Z' so that it gets four strokes, and we can have two pages with 4 rows).

chatcher

Defend usage of Math.min() in getLastRowIndex().
~~Address comma-separated var declarations.~~
Clarify calculation changes on getLastRowIndex()
Remove empty setShowing() function and related watcher.
Defend removal of showingLow and showingHigh.
Add more unit tests to ensure quality.

mportuga

Thank you for your changes!

chatcher closed this Feb 28, 2016

chatcher reopened this Feb 28, 2016

fix(pagination): Refactor 'getLastRowIndex' to call 'getFirstRowIndex'

13bf807

instead of calling reduce another time.

AgDude added needs: fix tests and removed needs: fix tests labels Jul 30, 2016

mportuga added the waiting-response label Sep 20, 2016

fix(pagination): off-by-one error

29fdb7c

page index and page number were confused.

mportuga requested changes Sep 25, 2016

View reviewed changes

chatcher added 2 commits September 25, 2016 12:13

chore(pagination): remove empty 'setShowing()' and related watcher

0d03099

chatcher commented Sep 25, 2016

View reviewed changes

mportuga approved these changes Sep 28, 2016

View reviewed changes

mportuga added Looks Good To Me and removed needs: fix tests waiting-response labels Sep 28, 2016

dlgski merged commit b35fca4 into angular-ui:master Nov 16, 2016

		@@ -368,8 +410,6 @@
		$scope.$on('$destroy', dataChangeDereg);

		var setShowing = function () {

feat(pagination): Add custom pagination with variable page sizes #5174

feat(pagination): Add custom pagination with variable page sizes #5174

Uh oh!

Conversation

chatcher commented Feb 28, 2016

Uh oh!

chatcher commented Feb 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AgDude commented Jul 30, 2016

Uh oh!

mportuga commented Sep 20, 2016

Uh oh!

mportuga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatcher left a comment

Choose a reason for hiding this comment

Uh oh!

mportuga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chatcher commented Feb 28, 2016 •

edited

Loading