From ffb44ffb94ada336f84c09fcac65354c89d3790d Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 7 Nov 2023 17:18:34 +0000 Subject: [PATCH 01/17] chore: revamp Page Utility tests --- .../components/PaginationWidget/utils.test.ts | 92 +++++++++++++------ site/src/components/PaginationWidget/utils.ts | 8 +- 2 files changed, 71 insertions(+), 29 deletions(-) diff --git a/site/src/components/PaginationWidget/utils.test.ts b/site/src/components/PaginationWidget/utils.test.ts index 01f79eb0a9f30..a73d994a41e01 100644 --- a/site/src/components/PaginationWidget/utils.test.ts +++ b/site/src/components/PaginationWidget/utils.test.ts @@ -1,29 +1,58 @@ import { buildPagedList, getOffset } from "./utils"; describe("buildPagedList", () => { - it.each<{ - numPages: number; - activePage: number; - expected: (string | number)[]; - }>([ - { numPages: 7, activePage: 1, expected: [1, 2, 3, 4, 5, 6, 7] }, - { numPages: 17, activePage: 1, expected: [1, 2, 3, 4, 5, "right", 17] }, - { - numPages: 17, - activePage: 9, - expected: [1, "left", 8, 9, 10, "right", 17], - }, - { - numPages: 17, - activePage: 17, - expected: [1, "left", 13, 14, 15, 16, 17], - }, - ])( - `buildPagedList($numPages, $activePage)`, - ({ numPages, activePage, expected }) => { - expect(buildPagedList(numPages, activePage)).toEqual(expected); - }, - ); + it("has no placeholder entries when there are seven or fewer pages", () => { + for (let i = 1; i <= 7; i++) { + const expectedResult: number[] = []; + for (let j = 1; j <= i; j++) { + expectedResult.push(j); + } + + expect(buildPagedList(i, i)).toEqual(expectedResult); + } + }); + + it("has 'right' placeholder for long lists when active page is near beginning", () => { + expect(buildPagedList(17, 1)).toEqual([1, 2, 3, 4, 5, "right", 17]); + }); + + it("has 'left' placeholder for long lists when active page is near end", () => { + expect(buildPagedList(17, 17)).toEqual([1, "left", 13, 14, 15, 16, 17]); + }); + + it("has both placeholders for long lists when active page is in the middle", () => { + expect(buildPagedList(17, 9)).toEqual([1, "left", 8, 9, 10, "right", 17]); + }); + + it("produces an empty array when there are no pages", () => { + expect(buildPagedList(0, 0)).toEqual([]); + }); + + it("makes sure all values are unique (for React rendering keys)", () => { + type TestEntry = [numPages: number, activePage: number]; + const testData: TestEntry[] = [ + [0, 0], + [1, 1], + [2, 2], + [3, 3], + [4, 4], + [5, 5], + [6, 6], + [7, 7], + + [10, 3], + [7, 1], + [17, 1], + [17, 9], + ]; + + for (const [numPages, activePage] of testData) { + const result = buildPagedList(numPages, activePage); + const uniqueCount = new Set(result).size; + + expect(uniqueCount).toEqual(result.length); + } + }); }); describe("getOffset", () => { @@ -32,9 +61,18 @@ describe("getOffset", () => { const limit = 10; expect(getOffset(page, limit)).toEqual(0); }); - it("returns the limit on page 2", () => { - const page = 2; - const limit = 10; - expect(getOffset(page, limit)).toEqual(limit); + + it("Returns the results for page 1 when input is invalid", () => { + const inputs = [0, -1, -Infinity, NaN, Infinity, 3.6, 7.4545435]; + + for (const input of inputs) { + expect(getOffset(input, 10)).toEqual(0); + } + }); + + it("Returns offset based on the current page for all pages after 1", () => { + expect(getOffset(2, 10)).toEqual(10); + expect(getOffset(3, 10)).toEqual(20); + expect(getOffset(4, 45)).toEqual(135); }); }); diff --git a/site/src/components/PaginationWidget/utils.ts b/site/src/components/PaginationWidget/utils.ts index 9575396e29595..993dc4b2b8e6f 100644 --- a/site/src/components/PaginationWidget/utils.ts +++ b/site/src/components/PaginationWidget/utils.ts @@ -57,8 +57,12 @@ export const buildPagedList = ( }; // pages count from 1 -export const getOffset = (page: number, limit: number): number => - (page - 1) * limit; +export const getOffset = (page: number, limit: number): number => { + const pageIsValid = Number.isInteger(page) && page >= 1; + const pageToUse = pageIsValid ? page : 1; + + return (pageToUse - 1) * limit; +}; export const nonInitialPage = (searchParams: URLSearchParams): boolean => { const page = searchParams.get("page"); From 98e43bec08233c5a99570f5703635f140bc106a0 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 7 Nov 2023 17:20:26 +0000 Subject: [PATCH 02/17] refactor: simplify component design for PageButton --- .../PaginationWidget/PageButton.tsx | 125 +++++++++++++----- .../PaginationWidget/PaginationWidgetBase.tsx | 100 +++++++++----- 2 files changed, 158 insertions(+), 67 deletions(-) diff --git a/site/src/components/PaginationWidget/PageButton.tsx b/site/src/components/PaginationWidget/PageButton.tsx index 9b89486359cfb..ceef484aa13b3 100644 --- a/site/src/components/PaginationWidget/PageButton.tsx +++ b/site/src/components/PaginationWidget/PageButton.tsx @@ -1,45 +1,108 @@ +import { PropsWithChildren } from "react"; import Button from "@mui/material/Button"; -import { css, useTheme } from "@emotion/react"; - -interface PageButtonProps { - activePage?: number; - page?: number; - placeholder?: string; - numPages?: number; - onPageClick?: (page: number) => void; +import { useTheme } from "@emotion/react"; + +type NumberedPageButtonProps = { + pageNumber: number; + totalPages: number; + + onClick?: () => void; + highlighted?: boolean; disabled?: boolean; +}; + +export function NumberedPageButton({ + pageNumber, + totalPages, + onClick, + highlighted = false, + disabled = false, +}: NumberedPageButtonProps) { + return ( + + {pageNumber} + + ); +} + +type PlaceholderPageButtonProps = PropsWithChildren<{ + pagesOmitted: number; +}>; + +export function PlaceholderPageButton({ + pagesOmitted, + children = <>…, +}: PlaceholderPageButtonProps) { + return ( + + {children} + + ); } -export const PageButton = ({ - activePage, - page, - placeholder = "...", - numPages, - onPageClick, +type BasePageButtonProps = PropsWithChildren<{ + name: string; + "aria-label": string; + + onClick?: () => void; + highlighted?: boolean; + disabled?: boolean; +}>; + +function BasePageButton({ + children, + onClick, + name, + "aria-label": ariaLabel, + highlighted = false, disabled = false, -}: PageButtonProps): JSX.Element => { +}: BasePageButtonProps) { const theme = useTheme(); + return ( ); -}; +} + +function getNumberedButtonLabel( + page: number, + totalPages: number, + highlighted: boolean, +): string { + if (highlighted) { + return "Current Page"; + } + + if (page === 1) { + return "First Page"; + } + + if (page === totalPages) { + return "Last Page"; + } + + return `Page ${page}`; +} diff --git a/site/src/components/PaginationWidget/PaginationWidgetBase.tsx b/site/src/components/PaginationWidget/PaginationWidgetBase.tsx index 56f3767859a78..ed95cde70096d 100644 --- a/site/src/components/PaginationWidget/PaginationWidgetBase.tsx +++ b/site/src/components/PaginationWidget/PaginationWidgetBase.tsx @@ -1,34 +1,35 @@ +import { type ReactElement } from "react"; import Button from "@mui/material/Button"; import useMediaQuery from "@mui/material/useMediaQuery"; import KeyboardArrowLeft from "@mui/icons-material/KeyboardArrowLeft"; import KeyboardArrowRight from "@mui/icons-material/KeyboardArrowRight"; import { useTheme } from "@emotion/react"; -import { PageButton } from "./PageButton"; +import { PlaceholderPageButton, NumberedPageButton } from "./PageButton"; import { buildPagedList } from "./utils"; export type PaginationWidgetBaseProps = { count: number; page: number; limit: number; - onChange: (page: number) => void; + onChange: (newPage: number) => void; }; export const PaginationWidgetBase = ({ count, - page, limit, onChange, -}: PaginationWidgetBaseProps): JSX.Element | null => { + page: currentPage, +}: PaginationWidgetBaseProps): ReactElement | null => { const theme = useTheme(); const isMobile = useMediaQuery(theme.breakpoints.down("md")); + const totalPages = Math.ceil(count / limit); - const numPages = Math.ceil(count / limit); - if (numPages < 2) { + if (totalPages < 2) { return null; } - const isFirstPage = page <= 1; - const isLastPage = page >= numPages; + const isFirstPage = currentPage <= 1; + const isLastPage = currentPage >= totalPages; return (
+ {isMobile ? ( - + ) : ( - buildPagedList(numPages, page).map((pageItem) => { - if (pageItem === "left" || pageItem === "right") { - return ( - - ); - } - - return ( - onChange(pageItem)} - /> - ); - }) + )} +
); }; + +type PaginationRowProps = { + currentPage: number; + totalPages: number; + onChange: (newPage: number) => void; +}; + +function PaginationRow({ + currentPage, + totalPages, + onChange, +}: PaginationRowProps) { + const pageInfo = buildPagedList(totalPages, currentPage); + const pagesOmitted = totalPages - pageInfo.length - 1; + + return ( + <> + {pageInfo.map((pageEntry) => { + if (pageEntry === "left" || pageEntry === "right") { + return ( + + ); + } + + return ( + onChange(pageEntry)} + /> + ); + })} + + ); +} From a08a14972626b3d3241527a80fa0ed1ec8035e94 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 7 Nov 2023 17:36:54 +0000 Subject: [PATCH 03/17] chore: beef up isNonInitialPage and add tests --- .../components/PaginationWidget/utils.test.ts | 36 ++++++++++++++++--- site/src/components/PaginationWidget/utils.ts | 8 +++-- site/src/pages/AuditPage/AuditPage.tsx | 4 +-- site/src/pages/UsersPage/UsersPage.tsx | 4 +-- 4 files changed, 41 insertions(+), 11 deletions(-) diff --git a/site/src/components/PaginationWidget/utils.test.ts b/site/src/components/PaginationWidget/utils.test.ts index a73d994a41e01..12c53af85dc75 100644 --- a/site/src/components/PaginationWidget/utils.test.ts +++ b/site/src/components/PaginationWidget/utils.test.ts @@ -1,6 +1,6 @@ -import { buildPagedList, getOffset } from "./utils"; +import { buildPagedList, getOffset, isNonInitialPage } from "./utils"; -describe("buildPagedList", () => { +describe(buildPagedList.name, () => { it("has no placeholder entries when there are seven or fewer pages", () => { for (let i = 1; i <= 7; i++) { const expectedResult: number[] = []; @@ -55,7 +55,7 @@ describe("buildPagedList", () => { }); }); -describe("getOffset", () => { +describe(getOffset.name, () => { it("returns 0 on page 1", () => { const page = 1; const limit = 10; @@ -70,9 +70,37 @@ describe("getOffset", () => { } }); - it("Returns offset based on the current page for all pages after 1", () => { + it("Returns offset based on the current page for all valid pages after 1", () => { expect(getOffset(2, 10)).toEqual(10); expect(getOffset(3, 10)).toEqual(20); expect(getOffset(4, 45)).toEqual(135); }); }); + +describe(isNonInitialPage.name, () => { + it("Should detect your page correctly if input is set and valid", () => { + const params1 = new URLSearchParams({ page: String(1) }); + expect(isNonInitialPage(params1)).toBe(false); + + const inputs = [2, 50, 500, 3722]; + + for (const input of inputs) { + const params = new URLSearchParams({ page: String(input) }); + expect(isNonInitialPage(params)).toBe(true); + } + }); + + it("Should act as if you are on page 1 if input is set but invalid", () => { + const inputs = ["", Infinity, -Infinity, NaN, 3.74, -3]; + + for (const input of inputs) { + const params = new URLSearchParams({ page: String(input) }); + expect(isNonInitialPage(params)).toBe(false); + } + }); + + it("Should act as if you are on page 1 if input does not exist", () => { + const params = new URLSearchParams(); + expect(isNonInitialPage(params)).toBe(false); + }); +}); diff --git a/site/src/components/PaginationWidget/utils.ts b/site/src/components/PaginationWidget/utils.ts index 993dc4b2b8e6f..eef1400a91fef 100644 --- a/site/src/components/PaginationWidget/utils.ts +++ b/site/src/components/PaginationWidget/utils.ts @@ -64,8 +64,10 @@ export const getOffset = (page: number, limit: number): number => { return (pageToUse - 1) * limit; }; -export const nonInitialPage = (searchParams: URLSearchParams): boolean => { +export const isNonInitialPage = (searchParams: URLSearchParams): boolean => { const page = searchParams.get("page"); - const numberPage = page ? Number(page) : 1; - return numberPage > 1; + const conversion = Number(page); + const inputIsValid = Number.isInteger(conversion) && conversion >= 1; + + return inputIsValid ? conversion > 1 : false; }; diff --git a/site/src/pages/AuditPage/AuditPage.tsx b/site/src/pages/AuditPage/AuditPage.tsx index 2ed69c608fdfe..174bf517a480c 100644 --- a/site/src/pages/AuditPage/AuditPage.tsx +++ b/site/src/pages/AuditPage/AuditPage.tsx @@ -1,6 +1,6 @@ import { DEFAULT_RECORDS_PER_PAGE, - nonInitialPage, + isNonInitialPage, } from "components/PaginationWidget/utils"; import { useFeatureVisibility } from "hooks/useFeatureVisibility"; import { FC } from "react"; @@ -73,7 +73,7 @@ const AuditPage: FC = () => { page={pagination.page} limit={pagination.limit} onPageChange={pagination.goToPage} - isNonInitialPage={nonInitialPage(searchParamsResult[0])} + isNonInitialPage={isNonInitialPage(searchParamsResult[0])} isAuditLogVisible={isAuditLogVisible} error={error} filterProps={{ diff --git a/site/src/pages/UsersPage/UsersPage.tsx b/site/src/pages/UsersPage/UsersPage.tsx index 7d09df412b32e..dd60039056f02 100644 --- a/site/src/pages/UsersPage/UsersPage.tsx +++ b/site/src/pages/UsersPage/UsersPage.tsx @@ -28,7 +28,7 @@ import { prepareQuery } from "utils/filters"; import { Helmet } from "react-helmet-async"; import { DeleteDialog } from "components/Dialogs/DeleteDialog/DeleteDialog"; -import { nonInitialPage } from "components/PaginationWidget/utils"; +import { isNonInitialPage } from "components/PaginationWidget/utils"; import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog"; import { ResetPasswordDialog } from "./ResetPasswordDialog"; import { pageTitle } from "utils/page"; @@ -157,7 +157,7 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { isLoading={isLoading} canEditUsers={canEditUsers} canViewActivity={entitlements.features.audit_log.enabled} - isNonInitialPage={nonInitialPage(searchParams)} + isNonInitialPage={isNonInitialPage(searchParams)} actorID={me.id} filterProps={{ filter: useFilterResult, From 183cdf720b5a00c33cdb583ab3a08113a5277dd4 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 7 Nov 2023 18:00:27 +0000 Subject: [PATCH 04/17] docs: clean up comments --- site/src/components/PaginationWidget/utils.ts | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/site/src/components/PaginationWidget/utils.ts b/site/src/components/PaginationWidget/utils.ts index eef1400a91fef..5c12e658100de 100644 --- a/site/src/components/PaginationWidget/utils.ts +++ b/site/src/components/PaginationWidget/utils.ts @@ -1,30 +1,35 @@ /** * Generates a ranged array with an option to step over values. * Shamelessly stolen from: - * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from#sequence_generator_range + * {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from#sequence_generator_range} */ const range = (start: number, stop: number, step = 1) => Array.from({ length: (stop - start) / step + 1 }, (_, i) => start + i * step); export const DEFAULT_RECORDS_PER_PAGE = 25; -// Number of pages to the left or right of the current page selection. + +// Number of pages to display on either side of the current page selection const PAGE_NEIGHBORS = 1; -// Number of pages displayed for cases where there are multiple ellipsis showing. This can be -// thought of as the minimum number of page numbers to display when multiple ellipsis are showing. + +// Minimum number of pages to display at all times (assuming there are enough +// pages). Needed to handle case where there are the left and right +// placeholder/ellipsis elements are both showing const PAGES_TO_DISPLAY = PAGE_NEIGHBORS * 2 + 3; -// Total page blocks(page numbers or ellipsis) displayed, including the maximum number of ellipsis (2). -// This gives us maximum number of 7 page blocks to be displayed when the page neighbors value is 1. -const NUM_PAGE_BLOCKS = PAGES_TO_DISPLAY + 2; + +// Total number of pagination elements to display (accounting for visible pages +// and up to two ellipses placeholders). With 1 page neighbor on either side, +// the UI will show up to seven elements total +const TOTAL_PAGE_BLOCKS = PAGES_TO_DISPLAY + 2; /** - * Builds a list of pages based on how many pages exist and where the user is in their navigation of those pages. - * List result is used to from the buttons that make up the Pagination Widget + * Takes the total number of pages from a pagination result, and truncates it + * into a UI-friendly list. */ export const buildPagedList = ( numPages: number, activePage: number, ): ("left" | "right" | number)[] => { - if (numPages > NUM_PAGE_BLOCKS) { + if (numPages > TOTAL_PAGE_BLOCKS) { let pages = []; const leftBound = activePage - PAGE_NEIGHBORS; const rightBound = activePage + PAGE_NEIGHBORS; @@ -56,12 +61,14 @@ export const buildPagedList = ( return range(1, numPages); }; -// pages count from 1 -export const getOffset = (page: number, limit: number): number => { +/** + * Calculates the current offset from the start of a paginated dataset + */ +export const getOffset = (page: number, pageSize: number): number => { const pageIsValid = Number.isInteger(page) && page >= 1; const pageToUse = pageIsValid ? page : 1; - return (pageToUse - 1) * limit; + return (pageToUse - 1) * pageSize; }; export const isNonInitialPage = (searchParams: URLSearchParams): boolean => { From 96553685614e6e696ab30476c94e168f3d3fc159 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 7 Nov 2023 18:02:54 +0000 Subject: [PATCH 05/17] chore: quick refactor for buildPagedList --- site/src/components/PaginationWidget/utils.ts | 47 +++++++++---------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/site/src/components/PaginationWidget/utils.ts b/site/src/components/PaginationWidget/utils.ts index 5c12e658100de..1f3422ba6f190 100644 --- a/site/src/components/PaginationWidget/utils.ts +++ b/site/src/components/PaginationWidget/utils.ts @@ -29,36 +29,35 @@ export const buildPagedList = ( numPages: number, activePage: number, ): ("left" | "right" | number)[] => { - if (numPages > TOTAL_PAGE_BLOCKS) { - let pages = []; - const leftBound = activePage - PAGE_NEIGHBORS; - const rightBound = activePage + PAGE_NEIGHBORS; - const beforeLastPage = numPages - 1; - const startPage = leftBound > 2 ? leftBound : 2; - const endPage = rightBound < beforeLastPage ? rightBound : beforeLastPage; + if (numPages <= TOTAL_PAGE_BLOCKS) { + return range(1, numPages); + } - pages = range(startPage, endPage); + const leftBound = activePage - PAGE_NEIGHBORS; + const rightBound = activePage + PAGE_NEIGHBORS; + const beforeLastPage = numPages - 1; + const startPage = leftBound > 2 ? leftBound : 2; + const endPage = rightBound < beforeLastPage ? rightBound : beforeLastPage; - const singleSpillOffset = PAGES_TO_DISPLAY - pages.length - 1; - const hasLeftOverflow = startPage > 2; - const hasRightOverflow = endPage < beforeLastPage; - const leftOverflowPage = "left" as const; - const rightOverflowPage = "right" as const; + let pages: ReturnType = range(startPage, endPage); - if (hasLeftOverflow && !hasRightOverflow) { - const extraPages = range(startPage - singleSpillOffset, startPage - 1); - pages = [leftOverflowPage, ...extraPages, ...pages]; - } else if (!hasLeftOverflow && hasRightOverflow) { - const extraPages = range(endPage + 1, endPage + singleSpillOffset); - pages = [...pages, ...extraPages, rightOverflowPage]; - } else if (hasLeftOverflow && hasRightOverflow) { - pages = [leftOverflowPage, ...pages, rightOverflowPage]; - } + const singleSpillOffset = PAGES_TO_DISPLAY - pages.length - 1; + const hasLeftOverflow = startPage > 2; + const hasRightOverflow = endPage < beforeLastPage; + const leftOverflowPage = "left"; + const rightOverflowPage = "right"; - return [1, ...pages, numPages]; + if (hasLeftOverflow && !hasRightOverflow) { + const extraPages = range(startPage - singleSpillOffset, startPage - 1); + pages = [leftOverflowPage, ...extraPages, ...pages]; + } else if (!hasLeftOverflow && hasRightOverflow) { + const extraPages = range(endPage + 1, endPage + singleSpillOffset); + pages = [...pages, ...extraPages, rightOverflowPage]; + } else if (hasLeftOverflow && hasRightOverflow) { + pages = [leftOverflowPage, ...pages, rightOverflowPage]; } - return range(1, numPages); + return [1, ...pages, numPages]; }; /** From cf89b9b6efa962fa6576a2b6bc5b8eee078a91c7 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 7 Nov 2023 18:12:58 +0000 Subject: [PATCH 06/17] refactor: clean up math calculations for buildPagedList --- site/src/components/PaginationWidget/utils.ts | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/site/src/components/PaginationWidget/utils.ts b/site/src/components/PaginationWidget/utils.ts index 1f3422ba6f190..75ecf721337a3 100644 --- a/site/src/components/PaginationWidget/utils.ts +++ b/site/src/components/PaginationWidget/utils.ts @@ -33,28 +33,24 @@ export const buildPagedList = ( return range(1, numPages); } - const leftBound = activePage - PAGE_NEIGHBORS; - const rightBound = activePage + PAGE_NEIGHBORS; - const beforeLastPage = numPages - 1; - const startPage = leftBound > 2 ? leftBound : 2; - const endPage = rightBound < beforeLastPage ? rightBound : beforeLastPage; + const pageBeforeLast = numPages - 1; + const startPage = Math.max(activePage - PAGE_NEIGHBORS, 2); + const endPage = Math.min(activePage + PAGE_NEIGHBORS, pageBeforeLast); let pages: ReturnType = range(startPage, endPage); const singleSpillOffset = PAGES_TO_DISPLAY - pages.length - 1; const hasLeftOverflow = startPage > 2; - const hasRightOverflow = endPage < beforeLastPage; - const leftOverflowPage = "left"; - const rightOverflowPage = "right"; + const hasRightOverflow = endPage < pageBeforeLast; if (hasLeftOverflow && !hasRightOverflow) { const extraPages = range(startPage - singleSpillOffset, startPage - 1); - pages = [leftOverflowPage, ...extraPages, ...pages]; + pages = ["left", ...extraPages, ...pages]; } else if (!hasLeftOverflow && hasRightOverflow) { const extraPages = range(endPage + 1, endPage + singleSpillOffset); - pages = [...pages, ...extraPages, rightOverflowPage]; + pages = [...pages, ...extraPages, "right"]; } else if (hasLeftOverflow && hasRightOverflow) { - pages = [leftOverflowPage, ...pages, rightOverflowPage]; + pages = ["left", ...pages, "right"]; } return [1, ...pages, numPages]; From edc0de912780522b082000eaa77d0de766cfed1a Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 7 Nov 2023 18:16:21 +0000 Subject: [PATCH 07/17] chore: rename PageButtons file --- .../PaginationWidget/{PageButton.tsx => PageButtons.tsx} | 0 site/src/components/PaginationWidget/PaginationWidgetBase.tsx | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename site/src/components/PaginationWidget/{PageButton.tsx => PageButtons.tsx} (100%) diff --git a/site/src/components/PaginationWidget/PageButton.tsx b/site/src/components/PaginationWidget/PageButtons.tsx similarity index 100% rename from site/src/components/PaginationWidget/PageButton.tsx rename to site/src/components/PaginationWidget/PageButtons.tsx diff --git a/site/src/components/PaginationWidget/PaginationWidgetBase.tsx b/site/src/components/PaginationWidget/PaginationWidgetBase.tsx index ed95cde70096d..485b684d1403d 100644 --- a/site/src/components/PaginationWidget/PaginationWidgetBase.tsx +++ b/site/src/components/PaginationWidget/PaginationWidgetBase.tsx @@ -4,7 +4,7 @@ import useMediaQuery from "@mui/material/useMediaQuery"; import KeyboardArrowLeft from "@mui/icons-material/KeyboardArrowLeft"; import KeyboardArrowRight from "@mui/icons-material/KeyboardArrowRight"; import { useTheme } from "@emotion/react"; -import { PlaceholderPageButton, NumberedPageButton } from "./PageButton"; +import { PlaceholderPageButton, NumberedPageButton } from "./PageButtons"; import { buildPagedList } from "./utils"; export type PaginationWidgetBaseProps = { From aef28e6023250b53c2a504721843c8afe56cd1d4 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 7 Nov 2023 19:38:02 +0000 Subject: [PATCH 08/17] chore: revamp how nav buttons are defined --- .../PaginationWidget/PaginationNavButtons.tsx | 124 ++++++++++++++++++ .../PaginationWidget/PaginationWidgetBase.tsx | 35 +---- 2 files changed, 129 insertions(+), 30 deletions(-) create mode 100644 site/src/components/PaginationWidget/PaginationNavButtons.tsx diff --git a/site/src/components/PaginationWidget/PaginationNavButtons.tsx b/site/src/components/PaginationWidget/PaginationNavButtons.tsx new file mode 100644 index 0000000000000..93eefd6ad05fd --- /dev/null +++ b/site/src/components/PaginationWidget/PaginationNavButtons.tsx @@ -0,0 +1,124 @@ +import { + type PropsWithChildren, + type ReactNode, + useEffect, + useState, +} from "react"; + +import KeyboardArrowLeft from "@mui/icons-material/KeyboardArrowLeft"; +import KeyboardArrowRight from "@mui/icons-material/KeyboardArrowRight"; +import Button from "@mui/material/Button"; +import Tooltip from "@mui/material/Tooltip"; +import { useTheme } from "@emotion/react"; + +type NavProps = { + currentPage: number; + onChange: (newPage: number) => void; +}; + +export function LeftNavButton({ currentPage, onChange }: NavProps) { + const isFirstPage = currentPage <= 1; + + return ( + { + if (!isFirstPage) { + onChange(currentPage - 1); + } + }} + > + + + ); +} + +export function RightNavButton({ currentPage, onChange }: NavProps) { + const isLastPage = currentPage <= 1; + + return ( + { + if (!isLastPage) { + onChange(currentPage + 1); + } + }} + > + + + ); +} + +type BaseButtonProps = PropsWithChildren<{ + disabled: boolean; + disabledMessage: ReactNode; + onClick: () => void; + "aria-label": string; + + disabledMessageTimeout?: number; +}>; + +function BaseNavButton({ + children, + onClick, + disabled, + disabledMessage, + "aria-label": ariaLabel, + disabledMessageTimeout = 3000, +}: BaseButtonProps) { + const theme = useTheme(); + const [showDisabledMessage, setShowDisabledMessage] = useState(false); + + useEffect(() => { + if (!showDisabledMessage) { + return; + } + + const timeoutId = window.setTimeout( + () => setShowDisabledMessage(false), + disabledMessageTimeout, + ); + + return () => window.clearTimeout(timeoutId); + }, [showDisabledMessage, disabledMessageTimeout]); + + // Using inline state sync to help MUI render accurately more quickly; same + // idea as the useEffect approach, but state changes flush faster + if (!disabled && showDisabledMessage) { + setShowDisabledMessage(false); + } + + return ( + + + + ); +} diff --git a/site/src/components/PaginationWidget/PaginationWidgetBase.tsx b/site/src/components/PaginationWidget/PaginationWidgetBase.tsx index 485b684d1403d..fea7f5af99ad7 100644 --- a/site/src/components/PaginationWidget/PaginationWidgetBase.tsx +++ b/site/src/components/PaginationWidget/PaginationWidgetBase.tsx @@ -1,11 +1,9 @@ -import { type ReactElement } from "react"; -import Button from "@mui/material/Button"; import useMediaQuery from "@mui/material/useMediaQuery"; -import KeyboardArrowLeft from "@mui/icons-material/KeyboardArrowLeft"; -import KeyboardArrowRight from "@mui/icons-material/KeyboardArrowRight"; import { useTheme } from "@emotion/react"; + import { PlaceholderPageButton, NumberedPageButton } from "./PageButtons"; import { buildPagedList } from "./utils"; +import { LeftNavButton, RightNavButton } from "./PaginationNavButtons"; export type PaginationWidgetBaseProps = { count: number; @@ -19,7 +17,7 @@ export const PaginationWidgetBase = ({ limit, onChange, page: currentPage, -}: PaginationWidgetBaseProps): ReactElement | null => { +}: PaginationWidgetBaseProps) => { const theme = useTheme(); const isMobile = useMediaQuery(theme.breakpoints.down("md")); const totalPages = Math.ceil(count / limit); @@ -28,9 +26,6 @@ export const PaginationWidgetBase = ({ return null; } - const isFirstPage = currentPage <= 1; - const isLastPage = currentPage >= totalPages; - return (
- + {isMobile ? ( )} - +
); }; From 58fdd2659e42eb0dbab8e43beb4c9a04059ffc73 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 7 Nov 2023 19:40:51 +0000 Subject: [PATCH 09/17] fix: remove test disabled state --- site/src/components/PaginationWidget/PaginationNavButtons.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/site/src/components/PaginationWidget/PaginationNavButtons.tsx b/site/src/components/PaginationWidget/PaginationNavButtons.tsx index 93eefd6ad05fd..cf2fb0e77baf2 100644 --- a/site/src/components/PaginationWidget/PaginationNavButtons.tsx +++ b/site/src/components/PaginationWidget/PaginationNavButtons.tsx @@ -96,7 +96,6 @@ function BaseNavButton({ return ( + {...delegatedProps} + /> ); } From eace4bf6f50a3a5fad798ccd98a341ea15183b9f Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 8 Nov 2023 22:03:50 +0000 Subject: [PATCH 17/17] fix: remove possible state sync bugs --- .../PaginationWidget/PaginationNavButton.tsx | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/site/src/components/PaginationWidget/PaginationNavButton.tsx b/site/src/components/PaginationWidget/PaginationNavButton.tsx index 9ddb7a54295d9..1bafe67f1fea8 100644 --- a/site/src/components/PaginationWidget/PaginationNavButton.tsx +++ b/site/src/components/PaginationWidget/PaginationNavButton.tsx @@ -11,7 +11,9 @@ import Tooltip from "@mui/material/Tooltip"; type PaginationNavButtonProps = Omit< ButtonHTMLAttributes, - "aria-disabled" | "color" + | "aria-disabled" + // Need to omit color for MUI compatibility + | "color" > & { // Required/narrowed versions of default props children: ReactNode; @@ -24,7 +26,7 @@ type PaginationNavButtonProps = Omit< disabledMessageTimeout?: number; }; -export function PaginationNavButton({ +function PaginationNavButtonCore({ onClick, disabled, disabledMessage, @@ -44,15 +46,12 @@ export function PaginationNavButton({ return; } - const timeoutId = window.setTimeout( + const timeoutId = setTimeout( () => setShowDisabledMessage(false), disabledMessageTimeout, ); - return () => { - setShowDisabledMessage(false); - window.clearTimeout(timeoutId); - }; + return () => clearTimeout(timeoutId); }, [showDisabledMessage, disabledMessageTimeout]); return ( @@ -89,3 +88,18 @@ export function PaginationNavButton({ ); } + +export function PaginationNavButton({ + disabledMessageTimeout = 3000, + ...delegatedProps +}: PaginationNavButtonProps) { + return ( + // Key prop ensures that if timeout changes, the component just unmounts and + // remounts, avoiding a swath of possible sync issues + + ); +}