Skip to content

Commit ad3abe3

Browse files
ParkreinerParkreiner
authored
refactor: revamp pagination UI view logic (#10567)
* chore: revamp Page Utility tests * refactor: simplify component design for PageButton * chore: beef up isNonInitialPage and add tests * docs: clean up comments * chore: quick refactor for buildPagedList * refactor: clean up math calculations for buildPagedList * chore: rename PageButtons file * chore: revamp how nav buttons are defined * fix: remove test disabled state * chore: clean up base nav button * chore: rename props for clarity * refactor: clean up logic for isNonInitialPage * chore: add more tests and catch bugs * docs: fix confusing typo in comments * chore: add one more test case for pagination buttons * refactor: update props definition for PaginationNavButton * fix: remove possible state sync bugs
1 parent 8a7f0e9 commit ad3abe3

13 files changed

+548
-193
lines changed

‎site/src/components/PaginationWidget/PageButton.tsx

-45
This file was deleted.

‎site/src/components/PaginationWidget/PageButtons.tsx

+108
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
import { PropsWithChildren } from "react";
2+
import Button from "@mui/material/Button";
3+
import { useTheme } from "@emotion/react";
4+
5+
type NumberedPageButtonProps = {
6+
pageNumber: number;
7+
totalPages: number;
8+
9+
onClick?: () => void;
10+
highlighted?: boolean;
11+
disabled?: boolean;
12+
};
13+
14+
export function NumberedPageButton({
15+
pageNumber,
16+
totalPages,
17+
onClick,
18+
highlighted = false,
19+
disabled = false,
20+
}: NumberedPageButtonProps) {
21+
return (
22+
<BasePageButton
23+
name="Page button"
24+
aria-label={getNumberedButtonLabel(pageNumber, totalPages, highlighted)}
25+
onClick={onClick}
26+
highlighted={highlighted}
27+
disabled={disabled}
28+
>
29+
{pageNumber}
30+
</BasePageButton>
31+
);
32+
}
33+
34+
type PlaceholderPageButtonProps = PropsWithChildren<{
35+
pagesOmitted: number;
36+
}>;
37+
38+
export function PlaceholderPageButton({
39+
pagesOmitted,
40+
children = <>&hellip;</>,
41+
}: PlaceholderPageButtonProps) {
42+
return (
43+
<BasePageButton
44+
disabled
45+
name="Omitted pages"
46+
aria-label={`Omitting ${pagesOmitted} pages`}
47+
>
48+
{children}
49+
</BasePageButton>
50+
);
51+
}
52+
53+
type BasePageButtonProps = PropsWithChildren<{
54+
name: string;
55+
"aria-label": string;
56+
57+
onClick?: () => void;
58+
highlighted?: boolean;
59+
disabled?: boolean;
60+
}>;
61+
62+
function BasePageButton({
63+
children,
64+
onClick,
65+
name,
66+
"aria-label": ariaLabel,
67+
highlighted = false,
68+
disabled = false,
69+
}: BasePageButtonProps) {
70+
const theme = useTheme();
71+
72+
return (
73+
<Button
74+
css={
75+
highlighted && {
76+
borderColor: `${theme.palette.info.main}`,
77+
backgroundColor: `${theme.palette.info.dark}`,
78+
}
79+
}
80+
aria-label={ariaLabel}
81+
name={name}
82+
disabled={disabled}
83+
onClick={onClick}
84+
>
85+
{children}
86+
</Button>
87+
);
88+
}
89+
90+
function getNumberedButtonLabel(
91+
page: number,
92+
totalPages: number,
93+
highlighted: boolean,
94+
): string {
95+
if (highlighted) {
96+
return "Current Page";
97+
}
98+
99+
if (page === 1) {
100+
return "First Page";
101+
}
102+
103+
if (page === totalPages) {
104+
return "Last Page";
105+
}
106+
107+
return `Page ${page}`;
108+
}

‎site/src/components/PaginationWidget/PaginationNavButton.tsx

+105
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
import {
2+
type ButtonHTMLAttributes,
3+
type ReactNode,
4+
useEffect,
5+
useState,
6+
} from "react";
7+
import { useTheme } from "@emotion/react";
8+
9+
import Button from "@mui/material/Button";
10+
import Tooltip from "@mui/material/Tooltip";
11+
12+
type PaginationNavButtonProps = Omit<
13+
ButtonHTMLAttributes<HTMLButtonElement>,
14+
| "aria-disabled"
15+
// Need to omit color for MUI compatibility
16+
| "color"
17+
> & {
18+
// Required/narrowed versions of default props
19+
children: ReactNode;
20+
disabled: boolean;
21+
onClick: () => void;
22+
"aria-label": string;
23+
24+
// Bespoke props
25+
disabledMessage: ReactNode;
26+
disabledMessageTimeout?: number;
27+
};
28+
29+
function PaginationNavButtonCore({
30+
onClick,
31+
disabled,
32+
disabledMessage,
33+
disabledMessageTimeout = 3000,
34+
...delegatedProps
35+
}: PaginationNavButtonProps) {
36+
const theme = useTheme();
37+
const [showDisabledMessage, setShowDisabledMessage] = useState(false);
38+
39+
// Inline state sync - this is safe/recommended by the React team in this case
40+
if (!disabled && showDisabledMessage) {
41+
setShowDisabledMessage(false);
42+
}
43+
44+
useEffect(() => {
45+
if (!showDisabledMessage) {
46+
return;
47+
}
48+
49+
const timeoutId = setTimeout(
50+
() => setShowDisabledMessage(false),
51+
disabledMessageTimeout,
52+
);
53+
54+
return () => clearTimeout(timeoutId);
55+
}, [showDisabledMessage, disabledMessageTimeout]);
56+
57+
return (
58+
<Tooltip title={disabledMessage} open={showDisabledMessage}>
59+
{/*
60+
* Going more out of the way to avoid attaching the disabled prop directly
61+
* to avoid unwanted side effects of using the prop:
62+
* - Not being focusable/keyboard-navigable
63+
* - Not being able to call functions in response to invalid actions
64+
* (mostly for giving direct UI feedback to those actions)
65+
*/}
66+
<Button
67+
aria-disabled={disabled}
68+
css={
69+
disabled && {
70+
borderColor: theme.palette.divider,
71+
color: theme.palette.text.disabled,
72+
cursor: "default",
73+
"&:hover": {
74+
backgroundColor: theme.palette.background.default,
75+
borderColor: theme.palette.divider,
76+
},
77+
}
78+
}
79+
onClick={() => {
80+
if (disabled) {
81+
setShowDisabledMessage(true);
82+
} else {
83+
onClick();
84+
}
85+
}}
86+
{...delegatedProps}
87+
/>
88+
</Tooltip>
89+
);
90+
}
91+
92+
export function PaginationNavButton({
93+
disabledMessageTimeout = 3000,
94+
...delegatedProps
95+
}: PaginationNavButtonProps) {
96+
return (
97+
// Key prop ensures that if timeout changes, the component just unmounts and
98+
// remounts, avoiding a swath of possible sync issues
99+
<PaginationNavButtonCore
100+
key={disabledMessageTimeout}
101+
disabledMessageTimeout={disabledMessageTimeout}
102+
{...delegatedProps}
103+
/>
104+
);
105+
}

‎site/src/components/PaginationWidget/PaginationWidgetBase.stories.tsx

+7-9
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ const meta: Meta<typeof PaginationWidgetBase> = {
55
title: "components/PaginationWidgetBase",
66
component: PaginationWidgetBase,
77
args: {
8-
page: 1,
9-
limit: 12,
10-
count: 200,
8+
currentPage: 1,
9+
pageSize: 12,
10+
totalRecords: 200,
1111
},
1212
};
1313

@@ -17,19 +17,17 @@ type Story = StoryObj<typeof PaginationWidgetBase>;
1717
export const MoreThan8Pages: Story = {};
1818

1919
export const LessThan8Pages: Story = {
20-
args: {
21-
count: 84,
22-
},
20+
args: { totalRecords: 84 },
2321
};
2422

2523
export const MoreThan7PagesWithActivePageCloseToStart: Story = {
26-
args: { page: 2, limit: 12 },
24+
args: { currentPage: 2, pageSize: 12 },
2725
};
2826

2927
export const MoreThan7PagesWithActivePageFarFromBoundaries: Story = {
30-
args: { page: 4, limit: 12 },
28+
args: { currentPage: 4, pageSize: 12 },
3129
};
3230

3331
export const MoreThan7PagesWithActivePageCloseToEnd: Story = {
34-
args: { page: 17, limit: 12 },
32+
args: { currentPage: 17, pageSize: 12 },
3533
};

‎site/src/components/PaginationWidget/PaginationWidgetBase.test.tsx

+86
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
import { screen } from "@testing-library/react";
2+
import {
3+
PaginationWidgetBase,
4+
PaginationWidgetBaseProps,
5+
} from "./PaginationWidgetBase";
6+
import { renderWithAuth } from "testHelpers/renderHelpers";
7+
import userEvent from "@testing-library/user-event";
8+
9+
type SampleProps = Omit<PaginationWidgetBaseProps, "onPageChange">;
10+
11+
describe(PaginationWidgetBase.name, () => {
12+
it("Should have its previous button be aria-disabled while on page 1", async () => {
13+
const sampleProps: SampleProps[] = [
14+
{ currentPage: 1, pageSize: 5, totalRecords: 6 },
15+
{ currentPage: 1, pageSize: 50, totalRecords: 200 },
16+
{ currentPage: 1, pageSize: 20, totalRecords: 3000 },
17+
];
18+
19+
for (const props of sampleProps) {
20+
const onPageChange = jest.fn();
21+
const { unmount } = renderWithAuth(
22+
<PaginationWidgetBase {...props} onPageChange={onPageChange} />,
23+
);
24+
25+
const prevButton = await screen.findByLabelText("Previous page");
26+
expect(prevButton).not.toBeDisabled();
27+
expect(prevButton).toHaveAttribute("aria-disabled", "true");
28+
29+
await userEvent.click(prevButton);
30+
expect(onPageChange).not.toHaveBeenCalled();
31+
unmount();
32+
}
33+
});
34+
35+
it("Should have its next button be aria-disabled while on last page", async () => {
36+
const sampleProps: SampleProps[] = [
37+
{ currentPage: 2, pageSize: 5, totalRecords: 6 },
38+
{ currentPage: 4, pageSize: 50, totalRecords: 200 },
39+
{ currentPage: 10, pageSize: 100, totalRecords: 1000 },
40+
];
41+
42+
for (const props of sampleProps) {
43+
const onPageChange = jest.fn();
44+
const { unmount } = renderWithAuth(
45+
<PaginationWidgetBase {...props} onPageChange={onPageChange} />,
46+
);
47+
48+
const button = await screen.findByLabelText("Next page");
49+
expect(button).not.toBeDisabled();
50+
expect(button).toHaveAttribute("aria-disabled", "true");
51+
52+
await userEvent.click(button);
53+
expect(onPageChange).not.toHaveBeenCalled();
54+
unmount();
55+
}
56+
});
57+
58+
it("Should have neither button be disabled for all other pages", async () => {
59+
const sampleProps: SampleProps[] = [
60+
{ currentPage: 11, pageSize: 5, totalRecords: 60 },
61+
{ currentPage: 2, pageSize: 50, totalRecords: 200 },
62+
{ currentPage: 3, pageSize: 20, totalRecords: 100 },
63+
];
64+
65+
for (const props of sampleProps) {
66+
const onPageChange = jest.fn();
67+
const { unmount } = renderWithAuth(
68+
<PaginationWidgetBase {...props} onPageChange={onPageChange} />,
69+
);
70+
71+
const prevButton = await screen.findByLabelText("Previous page");
72+
const nextButton = await screen.findByLabelText("Next page");
73+
74+
expect(prevButton).not.toBeDisabled();
75+
expect(prevButton).toHaveAttribute("aria-disabled", "false");
76+
77+
expect(nextButton).not.toBeDisabled();
78+
expect(nextButton).toHaveAttribute("aria-disabled", "false");
79+
80+
await userEvent.click(prevButton);
81+
await userEvent.click(nextButton);
82+
expect(onPageChange).toHaveBeenCalledTimes(2);
83+
unmount();
84+
}
85+
});
86+
});

0 commit comments

Comments
 (0)