Revise selection of pagination items

Revise the calculation of the set of page numbers so that the number of
pagination items (page numbers and elided-page markers) is consistent regardless
of the current page. If each item is rendered at a consistent width, this keeps
the controls in the same place as the user navigates through the page list,
avoiding mis-clicks due to pages jumping around.

src/components/navigation/Pagination.tsx

@@ -1,7 +1,7 @@
 import classnames from 'classnames';
 import type { ComponentChildren } from 'preact';
 
-import { pageNumberOptions } from '../../util/pagination';
+import { paginationItems } from '../../util/pagination';
 import { ArrowLeftIcon, ArrowRightIcon } from '../icons';
 import Button from '../input/Button';
 
@@ -75,7 +75,7 @@ export default function Pagination({
   // Pages are 1-indexed
   const hasNextPage = currentPage < totalPages;
   const hasPreviousPage = currentPage > 1;
-  const pageNumbers = pageNumberOptions(currentPage, totalPages);
+  const pageNumbers = paginationItems(currentPage, totalPages);
 
   const changePageTo = (pageNumber: number, element: HTMLElement) => {
     onChangePage(pageNumber);

src/components/navigation/test/Pagination-test.js

@@ -4,7 +4,7 @@ import Pagination, { $imports } from '../Pagination';
 
 describe('Pagination', () => {
   let fakeOnChangePage;
-  let fakePageNumberOptions;
+  let fakePaginationItems;
 
   const findButton = (wrapper, title) =>
     wrapper.find('button').filterWhere(n => n.props().title === title);
@@ -22,10 +22,10 @@ describe('Pagination', () => {
 
   beforeEach(() => {
     fakeOnChangePage = sinon.stub();
-    fakePageNumberOptions = sinon.stub().returns([1, 2, 3, 4, null, 10]);
+    fakePaginationItems = sinon.stub().returns([1, 2, 3, 4, null, 10]);
 
     $imports.$mock({
-      '../../util/pagination': { pageNumberOptions: fakePageNumberOptions },
+      '../../util/pagination': { paginationItems: fakePaginationItems },
     });
   });
 
@@ -103,7 +103,7 @@ describe('Pagination', () => {
 
   describe('page number buttons', () => {
     it('should render buttons for each page number available', () => {
-      fakePageNumberOptions.returns([1, 2, 3, 4, null, 10]);
+      fakePaginationItems.returns([1, 2, 3, 4, null, 10]);
       const wrapper = createComponent();
 
       [1, 2, 3, 4, 10].forEach(pageNumber => {
@@ -116,7 +116,7 @@ describe('Pagination', () => {
     });
 
     it('should invoke the onChangePage callback when page number button clicked', () => {
-      fakePageNumberOptions.returns([1, 2, 3, 4, null, 10]);
+      fakePaginationItems.returns([1, 2, 3, 4, null, 10]);
       const wrapper = createComponent();
 
       [1, 2, 3, 4, 10].forEach(pageNumber => {

src/util/pagination.ts

@@ -4,70 +4,145 @@
  */
 type PageNumber = number | null;
 
+export type Options = {
+  /**
+   * Number of pages to display at the start and end, including the start/end
+   * page.
+   *
+   * This must be >= 1.
+   */
+  boundaryCount?: number;
+
+  /**
+   * Number of pages to display before and after the current page.
+   */
+  siblingCount?: number;
+};
+
 /**
  * Determine the set of (pagination) page numbers that should be provided to
- * a user, given the current page the user is on, the total number of pages
- * available, and the number of individual page options desired.
+ * a user.
  *
- * The first, last and current pages will always be included in the returned
- * results. Additional pages adjacent to the current page will be added until
- * `maxPages` is reached. Gaps in the sequence of pages are represented by
- * `null` values.
+ * The result includes a mixture of page numbers that should be shown, plus
+ * `null` values indicating elided page numbers. The goals of the selection
+ * are:
  *
- * @example
- *   pageNumberOptions(1, 10, 5) => [1, 2, 3, 4, null, 10]
- *   pageNumberOptions(3, 10, 5) => [1, 2, 3, 4, null, 10]
- *   pageNumberOptions(6, 10, 5) => [1, null, 5, 6, 7, null, 10]
- *   pageNumberOptions(9, 10, 5) => [1, null, 7, 8, 9, 10]
- *   pageNumberOptions(2, 3, 5) => [1, 2, 3]
+ * - To always provide page numbers for the first, last and current pages.
+ *   Additional adjacent pages are provided according to the `boundaryCount`
+ *   and `siblingCount` options.
+ * - To try and keep the number of pagination items consistent as the current
+ *   page changes. If each item is rendered with approximately the same width,
+ *   this keeps the overall width of the pagination component and the location
+ *   of child controls consistent as the user navigates. This helps to avoid
+ *   mis-clicks due to controls moving around under the cursor.
  *
- * @param currentPage - The currently-visible/-active page of results.
- *   Note that pages are 1-indexed
- * @param maxPages - The maximum number of numbered pages to make available
- * @return Set of navigation page options to show. `null` values represent gaps
- *   in the sequence of pages, to be represented later as ellipses (...)
+ * @param currentPage - The 1-based currently-visible/-active page number.
+ * @param totalPages - The total number of pages
+ * @param options - Options for the number of pages to show at the boundary and
+ *   around the current page.
  */
-export function pageNumberOptions(
+export function paginationItems(
   currentPage: number,
   totalPages: number,
   /* istanbul ignore next */
-  maxPages = 5,
+  { boundaryCount = 1, siblingCount = 1 }: Options = {},
 ): PageNumber[] {
   if (totalPages <= 1) {
     return [];
   }
 
-  // Start with first, last and current page. Use a set to avoid dupes.
-  const pageNumbers = new Set([1, currentPage, totalPages]);
+  currentPage = Math.max(1, Math.min(currentPage, totalPages));
+  boundaryCount = Math.max(boundaryCount, 1);
+  siblingCount = Math.max(siblingCount, 0);
+
+  const pageNumbers: PageNumber[] = [];
+  const beforeCurrent = currentPage - 1;
+  const afterCurrent = totalPages - currentPage;
+
+  // Index of indicator for pages elided before current.
+  let elideBeforeIndex = null;
+  // Number of last page elided before current.
+  let elideBeforeValue = 1;
 
-  // Fill out the `pageNumbers` with additional pages near the currentPage,
-  // if available
-  let increment = 1;
-  while (pageNumbers.size < Math.min(totalPages, maxPages)) {
-    // Build the set "outward" from the currently-active page
-    if (currentPage + increment <= totalPages) {
-      pageNumbers.add(currentPage + increment);
+  // Add pages before current.
+  const elideBeforeCurrent = boundaryCount + siblingCount < beforeCurrent;
+  if (elideBeforeCurrent) {
+    for (let page = 1; page <= boundaryCount; page++) {
+      pageNumbers.push(page);
     }
-    if (currentPage - increment >= 1) {
-      pageNumbers.add(currentPage - increment);
+    elideBeforeIndex = pageNumbers.length;
+    elideBeforeValue = currentPage - siblingCount - 1;
+    pageNumbers.push(null);
+    for (let page = currentPage - siblingCount; page < currentPage; page++) {
+      pageNumbers.push(page);
+    }
+  } else {
+    for (let page = 1; page < currentPage; page++) {
+      pageNumbers.push(page);
     }
-    ++increment;
   }
 
-  const pageOptions: PageNumber[] = [];
+  pageNumbers.push(currentPage);
+
+  // Index of indicator for pages elided after current.
+  let elideAfterIndex = null;
+  // Number of first page elided after current.
+  let elideAfterValue = currentPage;
+
+  const elideAfterCurrent = boundaryCount + siblingCount < afterCurrent;
+  if (elideAfterCurrent) {
+    for (
+      let page = currentPage + 1;
+      page <= currentPage + siblingCount;
+      page++
+    ) {
+      pageNumbers.push(page);
+      elideAfterValue = page + 1;
+    }
+    elideAfterIndex = pageNumbers.length;
+    pageNumbers.push(null);
+    for (
+      let page = totalPages - boundaryCount + 1;
+      page <= totalPages;
+      page++
+    ) {
+      pageNumbers.push(page);
+    }
+  } else {
+    for (let page = currentPage + 1; page <= totalPages; page++) {
+      pageNumbers.push(page);
+    }
+  }
+
+  // Calculate the maximum number of items we will show for the total number
+  // of pages and options.
+  const maxItems = Math.min(
+    // First and last pages
+    2 * boundaryCount +
+      // Pages adjacent to current page
+      2 * siblingCount +
+      // Current page, indicators for elided pages before and after current.
+      3,
+    totalPages,
+  );
+
+  // To keep the number of items consistent as the current page changes,
+  // expand the elided ranges until we reach the maximum.
+  while (pageNumbers.length < maxItems) {
+    if (elideAfterIndex !== null) {
+      // Expand ahead of current page if possible, starting with numbers closest
+      // to current page.
+      pageNumbers.splice(elideAfterIndex, 0, elideAfterValue);
+      ++elideAfterIndex;
+      ++elideAfterValue;
+    } else if (elideBeforeIndex !== null) {
+      // Otherwise expand behind, starting with numbers closest to current page.
+      pageNumbers.splice(elideBeforeIndex + 1, 0, elideBeforeValue);
+      --elideBeforeValue;
+    } else {
+      break;
+    }
+  }
 
-  // Construct a numerically-sorted array with `null` entries inserted
-  // between non-sequential entries
-  [...pageNumbers]
-    .sort((a, b) => a - b)
-    .forEach((page, idx, arr) => {
-      if (idx > 0 && page - arr[idx - 1] > 1) {
-        // Two page entries are non-sequential. Push a `null` value between
-        // them to indicate the gap, which will later be represented as an
-        // ellipsis
-        pageOptions.push(null);
-      }
-      pageOptions.push(page);
-    });
-  return pageOptions;
+  return pageNumbers;
 }

src/util/test/pagination-test.js

@@ -1,22 +1,115 @@
-import { pageNumberOptions } from '../pagination';
-
-describe('sidebar/util/pagination', () => {
-  describe('pageNumberOptions', () => {
-    [
-      { args: [1, 10, 5], expected: [1, 2, 3, 4, null, 10] },
-      { args: [3, 10, 5], expected: [1, 2, 3, 4, null, 10] },
-      { args: [6, 10, 5], expected: [1, null, 5, 6, 7, null, 10] },
-      { args: [9, 10, 5], expected: [1, null, 7, 8, 9, 10] },
-      { args: [2, 3, 5], expected: [1, 2, 3] },
-      { args: [3, 10, 7], expected: [1, 2, 3, 4, 5, 6, null, 10] },
-      { args: [1, 1, 5], expected: [] },
-    ].forEach(testCase => {
-      it('should produce expected available page numbers', () => {
-        assert.deepEqual(
-          pageNumberOptions(...testCase.args),
-          testCase.expected,
-        );
+import { paginationItems } from '../pagination';
+
+describe('paginationItems', () => {
+  [
+    // Only one page
+    {
+      current: 1,
+      total: 1,
+      expected: [],
+    },
+
+    // No pages elided.
+    {
+      current: 1,
+      total: 3,
+      expected: [1, 2, 3],
+    },
+
+    // Pages elided after current.
+    {
+      current: 1,
+      total: 4,
+      expected: [1, 2, null, 4],
+    },
+
+    // Pages elided before current.
+    {
+      current: 4,
+      total: 6,
+      expected: [1, null, 3, 4, 5, 6],
+    },
+
+    // Pages elided before and after current.
+    {
+      current: 4,
+      total: 7,
+      expected: [1, null, 3, 4, 5, null, 7],
+    },
+
+    // Custom boundary count
+    {
+      current: 6,
+      total: 10,
+      boundaryCount: 2,
+      expected: [1, 2, null, 5, 6, 7, null, 9, 10],
+    },
+
+    // Custom sibling count
+    {
+      current: 6,
+      total: 10,
+      siblingCount: 2,
+      expected: [1, null, 4, 5, 6, 7, 8, null, 10],
+    },
+  ].forEach(({ current, total, boundaryCount, siblingCount, expected }) => {
+    it('should produce expected items', () => {
+      const items = paginationItems(current, total, {
+        boundaryCount,
+        siblingCount,
       });
+      assert.deepEqual(items, expected);
     });
   });
+
+  function* configurations() {
+    for (let boundaryCount = 1; boundaryCount <= 3; boundaryCount++) {
+      for (let siblingCount = 0; siblingCount <= 2; siblingCount++) {
+        for (let total = 1; total <= 10; total++) {
+          yield { total, boundaryCount, siblingCount };
+        }
+      }
+    }
+  }
+
+  function isSorted(list) {
+    return list.every((item, idx) => idx === 0 || item >= list[idx - 1]);
+  }
+
+  it('should produce expected items for generated configurations', () => {
+    for (const { total, boundaryCount, siblingCount } of configurations()) {
+      let expectedItems = null;
+      for (let current = 1; current <= total; current++) {
+        const items = paginationItems(current, total, {
+          boundaryCount,
+          siblingCount,
+        });
+
+        if (total === 1) {
+          assert.deepEqual(items, []);
+          continue;
+        }
+
+        // Total number of items should always be the same for a given total
+        // page count and configuration.
+        if (expectedItems === null) {
+          expectedItems = items.length;
+        } else {
+          assert.equal(items.length, expectedItems);
+        }
+
+        // Check that first, current and last pages are present.
+        assert.include(items, 1);
+        assert.include(items, current);
+        assert.include(items, total);
+
+        // There should be at most two elided page indicators
+        assert.isAtMost(items.filter(it => it === null).length, 2);
+
+        // Page numbers should always be in sorted order.
+        const numbers = items.filter(it => typeof it === 'number');
+        assert.isTrue(isSorted(numbers), `${numbers.join(', ')} is not sorted`);
+      }
+    }
+  });
 });