From 70a18053a5ba702f5e84118212f31074822172a9 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Mon, 9 Dec 2024 10:39:28 +0000 Subject: [PATCH 1/6] Remove background color from non-active pages in Pagination This is a vestige from when this component was first created for use in the client's Notebook UI. The background color for non-active pages matched the client's background color. --- src/components/navigation/Pagination.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/navigation/Pagination.tsx b/src/components/navigation/Pagination.tsx index f543905e..1ab2a310 100644 --- a/src/components/navigation/Pagination.tsx +++ b/src/components/navigation/Pagination.tsx @@ -18,7 +18,7 @@ function NavigationButton({ ...buttonProps }: NavigationButtonProps) { 'px-3.5 py-2.5 gap-x-1', 'font-semibold rounded', // These colors are the same as the "dark" variant of IconButton - 'text-grey-7 bg-grey-2 enabled:hover:text-grey-9 enabled:hover:bg-grey-3', + 'text-grey-7 enabled:hover:text-grey-9 enabled:hover:bg-grey-3', 'disabled:text-grey-5 aria-pressed:bg-grey-3 aria-expanded:bg-grey-3', )} {...buttonProps} From f691edf85b15caec981ae473caf4ae6bc89b0519 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Mon, 9 Dec 2024 10:40:37 +0000 Subject: [PATCH 2/6] Refine styling of elided pages indicator - Use Unicode ellipsis instead of ASCII "..." - Make the elided pages indicator approximately the same width as a small page number. Together with some future changes to how pagination items are generated, this will reduce the amount of variation in width of the Pagination component as the current page is advanced. --- src/components/navigation/Pagination.tsx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/components/navigation/Pagination.tsx b/src/components/navigation/Pagination.tsx index 1ab2a310..383f81c2 100644 --- a/src/components/navigation/Pagination.tsx +++ b/src/components/navigation/Pagination.tsx @@ -109,7 +109,15 @@ export default function Pagination({ {pageNumbers.map((page, idx) => (
  • {page === null ? ( -
    ...
    + // Indicator for elided pages. Should be approximately the same + // width as a small page number. This reduces the variation of + // the component's width as the current page is advanced. + // + // Navigation buttons have `px-3.5`. This uses `px-3` since + // an ellipsis is slightly wider than a digit. +
    + … +
    ) : ( Date: Mon, 9 Dec 2024 10:45:10 +0000 Subject: [PATCH 3/6] Disable text selection in Pagination controls This fixes an issue where repeatedly pressing the prev/next page buttons would sometimes cause the text to be selected. --- src/components/navigation/Pagination.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/navigation/Pagination.tsx b/src/components/navigation/Pagination.tsx index 383f81c2..4f01d654 100644 --- a/src/components/navigation/Pagination.tsx +++ b/src/components/navigation/Pagination.tsx @@ -73,7 +73,7 @@ export default function Pagination({ return (
    From 952971c5568c32a3ee6d5ca3accab7d8b178a2e0 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Mon, 9 Dec 2024 10:47:46 +0000 Subject: [PATCH 4/6] Remove fixed heights from Prev/Next page buttons This fixes an issue where the Prev button was not vertically aligned with the page numbers if the font size was larger than the one used in the client, where this component originates. --- src/components/navigation/Pagination.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/navigation/Pagination.tsx b/src/components/navigation/Pagination.tsx index 4f01d654..9afb0498 100644 --- a/src/components/navigation/Pagination.tsx +++ b/src/components/navigation/Pagination.tsx @@ -76,7 +76,7 @@ export default function Pagination({ className="flex items-center text-md select-none" data-testid="pagination-navigation" > -
    +
    {hasPreviousPage && (
    Date: Mon, 9 Dec 2024 11:03:03 +0000 Subject: [PATCH 5/6] Hide Prev/Next page buttons when unavailable instead of removing them This avoids the need to specify a fixed width for the container of the Prev/Next page buttons in order to keep other controls in a consistent position as the user navigates between pages, and the Prev/Next page buttons are shown/hidden. --- src/components/navigation/Pagination.tsx | 79 +++++++++++-------- .../navigation/test/Pagination-test.js | 20 +++-- 2 files changed, 58 insertions(+), 41 deletions(-) diff --git a/src/components/navigation/Pagination.tsx b/src/components/navigation/Pagination.tsx index 9afb0498..ae37cee4 100644 --- a/src/components/navigation/Pagination.tsx +++ b/src/components/navigation/Pagination.tsx @@ -1,17 +1,25 @@ import classnames from 'classnames'; -import type { JSX } from 'preact'; +import type { ComponentChildren } from 'preact'; -import type { PresentationalProps } from '../../types'; import { pageNumberOptions } from '../../util/pagination'; import { ArrowLeftIcon, ArrowRightIcon } from '../icons'; import Button from '../input/Button'; -import type { ButtonProps } from '../input/Button'; -type NavigationButtonProps = PresentationalProps & - ButtonProps & - Omit, 'icon' | 'size'>; +type NavigationButtonProps = { + children: ComponentChildren; + invisible?: boolean; + onClick: (e: MouseEvent) => void; + pressed?: boolean; + title?: string; +}; -function NavigationButton({ ...buttonProps }: NavigationButtonProps) { +function NavigationButton({ + children, + invisible = false, + pressed = false, + onClick, + title, +}: NavigationButtonProps) { return ( ); } @@ -76,18 +95,15 @@ export default function Pagination({ className="flex items-center text-md select-none" data-testid="pagination-navigation" > -
    - {hasPreviousPage && ( - - changePageTo(currentPage - 1, e.target as HTMLElement) - } - > - - prev - - )} +
    + changePageTo(currentPage - 1, e.target as HTMLElement)} + > + + prev +
      - {hasNextPage && ( - - changePageTo(currentPage + 1, e.target as HTMLElement) - } - > - next - - - )} + changePageTo(currentPage + 1, e.target as HTMLElement)} + > + next + +
    ); diff --git a/src/components/navigation/test/Pagination-test.js b/src/components/navigation/test/Pagination-test.js index 53abb36e..4d5f90b8 100644 --- a/src/components/navigation/test/Pagination-test.js +++ b/src/components/navigation/test/Pagination-test.js @@ -34,16 +34,18 @@ describe('Pagination', () => { }); describe('prev button', () => { - it('should render a prev button when there are previous pages to show', () => { + it('should render enabled prev button when there are previous pages to show', () => { const wrapper = createComponent({ currentPage: 2 }); const button = findButton(wrapper, 'Go to previous page'); - assert.isTrue(button.exists()); + assert.isFalse(button.find('button').hasClass('invisible')); + assert.isFalse(button.prop('disabled')); }); - it('should not render a prev button if there are no previous pages to show', () => { + it('should render disabled prev button if there are no previous pages to show', () => { const wrapper = createComponent({ currentPage: 1 }); const button = findButton(wrapper, 'Go to previous page'); - assert.isFalse(button.exists()); + assert.isTrue(button.find('button').hasClass('invisible')); + assert.isTrue(button.prop('disabled')); }); it('should invoke the onChangePage callback when clicked', () => { @@ -66,16 +68,18 @@ describe('Pagination', () => { }); describe('next button', () => { - it('should render a next button when there are further pages to show', () => { + it('should render enabled button when there are further pages to show', () => { const wrapper = createComponent({ currentPage: 1 }); const button = findButton(wrapper, 'Go to next page'); - assert.isTrue(button.exists()); + assert.isFalse(button.find('button').hasClass('invisible')); + assert.isFalse(button.prop('disabled')); }); - it('should not render a next button if there are no further pages to show', () => { + it('should render disabled next button if there are no further pages to show', () => { const wrapper = createComponent({ currentPage: 10 }); const button = findButton(wrapper, 'Go to next page'); - assert.isFalse(button.exists()); + assert.isTrue(button.find('button').hasClass('invisible')); + assert.isTrue(button.prop('disabled')); }); it('should invoke the `onChangePage` callback when clicked', () => { From b802d7a6ee7bc92c47d220cd35992680c4a54730 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Mon, 9 Dec 2024 11:09:53 +0000 Subject: [PATCH 6/6] Remove obsolete JSDoc types --- src/components/navigation/Pagination.tsx | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/components/navigation/Pagination.tsx b/src/components/navigation/Pagination.tsx index ae37cee4..a8514239 100644 --- a/src/components/navigation/Pagination.tsx +++ b/src/components/navigation/Pagination.tsx @@ -77,10 +77,6 @@ export default function Pagination({ const hasPreviousPage = currentPage > 1; const pageNumbers = pageNumberOptions(currentPage, totalPages); - /** - * @param {number} pageNumber - * @param {HTMLElement} element - */ const changePageTo = (pageNumber: number, element: HTMLElement) => { onChangePage(pageNumber); // Because changing pagination page doesn't reload the page (as it would