Skip to content

refactor: revamp pagination UI view logic #10567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Nov 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 0 additions & 45 deletions site/src/components/PaginationWidget/PageButton.tsx

This file was deleted.

108 changes: 108 additions & 0 deletions site/src/components/PaginationWidget/PageButtons.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import { PropsWithChildren } from "react";
import Button from "@mui/material/Button";
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 (
<BasePageButton
name="Page button"
aria-label={getNumberedButtonLabel(pageNumber, totalPages, highlighted)}
onClick={onClick}
highlighted={highlighted}
disabled={disabled}
>
{pageNumber}
</BasePageButton>
);
}

type PlaceholderPageButtonProps = PropsWithChildren<{
pagesOmitted: number;
}>;

export function PlaceholderPageButton({
pagesOmitted,
children = <>&hellip;</>,
}: PlaceholderPageButtonProps) {
return (
<BasePageButton
disabled
name="Omitted pages"
aria-label={`Omitting ${pagesOmitted} pages`}
>
{children}
</BasePageButton>
);
}

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,
}: BasePageButtonProps) {
const theme = useTheme();

return (
<Button
css={
highlighted && {
borderColor: `${theme.palette.info.main}`,
backgroundColor: `${theme.palette.info.dark}`,
}
}
aria-label={ariaLabel}
name={name}
disabled={disabled}
onClick={onClick}
>
{children}
</Button>
);
}

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}`;
}
105 changes: 105 additions & 0 deletions site/src/components/PaginationWidget/PaginationNavButton.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import {
type ButtonHTMLAttributes,
type ReactNode,
useEffect,
useState,
} from "react";
import { useTheme } from "@emotion/react";

import Button from "@mui/material/Button";
import Tooltip from "@mui/material/Tooltip";

type PaginationNavButtonProps = Omit<
ButtonHTMLAttributes<HTMLButtonElement>,
| "aria-disabled"
// Need to omit color for MUI compatibility
| "color"
> & {
// Required/narrowed versions of default props
children: ReactNode;
disabled: boolean;
onClick: () => void;
"aria-label": string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than special case this one prop, we should probably just extend HTMLAttributes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can extend it, but the thing is that even if I extend HTMLAttributes or componentPropsWithoutRef, I'll still need to treat aria-label as a special case to enforce that it's required

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh, fair enough.


// Bespoke props
disabledMessage: ReactNode;
disabledMessageTimeout?: number;
};

function PaginationNavButtonCore({
onClick,
disabled,
disabledMessage,
disabledMessageTimeout = 3000,
...delegatedProps
}: PaginationNavButtonProps) {
const theme = useTheme();
const [showDisabledMessage, setShowDisabledMessage] = useState(false);

// Inline state sync - this is safe/recommended by the React team in this case
if (!disabled && showDisabledMessage) {
setShowDisabledMessage(false);
}
Comment on lines +40 to +42
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like it 💀

Copy link
Member Author

@Parkreiner Parkreiner Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I remember seeing an article about it called something like "the ugliest pattern in React" or something.

It is better than using useEffect from a performance standpoint, and I guess also "philosophy" reasons (the React team is getting more bullish about how useEffect should be used strictly to sync with external systems, and if the state already exists inside React, you should do things like this)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and yet we also have the wonderful useSyncExternalStore hook :lolsob:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I get that the hook was mainly designed to help state management tools hook into React, but yeah. React's gonna React


useEffect(() => {
if (!showDisabledMessage) {
return;
}

const timeoutId = setTimeout(
() => setShowDisabledMessage(false),
disabledMessageTimeout,
);

return () => clearTimeout(timeoutId);
}, [showDisabledMessage, disabledMessageTimeout]);

return (
<Tooltip title={disabledMessage} open={showDisabledMessage}>
{/*
* Going more out of the way to avoid attaching the disabled prop directly
* to avoid unwanted side effects of using the prop:
* - Not being focusable/keyboard-navigable
* - Not being able to call functions in response to invalid actions
* (mostly for giving direct UI feedback to those actions)
*/}
<Button
aria-disabled={disabled}
css={
disabled && {
borderColor: theme.palette.divider,
color: theme.palette.text.disabled,
cursor: "default",
"&:hover": {
backgroundColor: theme.palette.background.default,
borderColor: theme.palette.divider,
},
}
}
onClick={() => {
if (disabled) {
setShowDisabledMessage(true);
} else {
onClick();
}
}}
{...delegatedProps}
/>
</Tooltip>
);
}

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
<PaginationNavButtonCore
key={disabledMessageTimeout}
disabledMessageTimeout={disabledMessageTimeout}
{...delegatedProps}
/>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ const meta: Meta<typeof PaginationWidgetBase> = {
title: "components/PaginationWidgetBase",
component: PaginationWidgetBase,
args: {
page: 1,
limit: 12,
count: 200,
currentPage: 1,
pageSize: 12,
totalRecords: 200,
},
};

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

export const LessThan8Pages: Story = {
args: {
count: 84,
},
args: { totalRecords: 84 },
};

export const MoreThan7PagesWithActivePageCloseToStart: Story = {
args: { page: 2, limit: 12 },
args: { currentPage: 2, pageSize: 12 },
};

export const MoreThan7PagesWithActivePageFarFromBoundaries: Story = {
args: { page: 4, limit: 12 },
args: { currentPage: 4, pageSize: 12 },
};

export const MoreThan7PagesWithActivePageCloseToEnd: Story = {
args: { page: 17, limit: 12 },
args: { currentPage: 17, pageSize: 12 },
};
86 changes: 86 additions & 0 deletions site/src/components/PaginationWidget/PaginationWidgetBase.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { screen } from "@testing-library/react";
import {
PaginationWidgetBase,
PaginationWidgetBaseProps,
} from "./PaginationWidgetBase";
import { renderWithAuth } from "testHelpers/renderHelpers";
import userEvent from "@testing-library/user-event";

type SampleProps = Omit<PaginationWidgetBaseProps, "onPageChange">;

describe(PaginationWidgetBase.name, () => {
it("Should have its previous button be aria-disabled while on page 1", async () => {
const sampleProps: SampleProps[] = [
{ currentPage: 1, pageSize: 5, totalRecords: 6 },
{ currentPage: 1, pageSize: 50, totalRecords: 200 },
{ currentPage: 1, pageSize: 20, totalRecords: 3000 },
];

for (const props of sampleProps) {
const onPageChange = jest.fn();
const { unmount } = renderWithAuth(
<PaginationWidgetBase {...props} onPageChange={onPageChange} />,
);

const prevButton = await screen.findByLabelText("Previous page");
expect(prevButton).not.toBeDisabled();
expect(prevButton).toHaveAttribute("aria-disabled", "true");

await userEvent.click(prevButton);
expect(onPageChange).not.toHaveBeenCalled();
unmount();
}
});

it("Should have its next button be aria-disabled while on last page", async () => {
const sampleProps: SampleProps[] = [
{ currentPage: 2, pageSize: 5, totalRecords: 6 },
{ currentPage: 4, pageSize: 50, totalRecords: 200 },
{ currentPage: 10, pageSize: 100, totalRecords: 1000 },
];

for (const props of sampleProps) {
const onPageChange = jest.fn();
const { unmount } = renderWithAuth(
<PaginationWidgetBase {...props} onPageChange={onPageChange} />,
);

const button = await screen.findByLabelText("Next page");
expect(button).not.toBeDisabled();
expect(button).toHaveAttribute("aria-disabled", "true");

await userEvent.click(button);
expect(onPageChange).not.toHaveBeenCalled();
unmount();
}
});

it("Should have neither button be disabled for all other pages", async () => {
const sampleProps: SampleProps[] = [
{ currentPage: 11, pageSize: 5, totalRecords: 60 },
{ currentPage: 2, pageSize: 50, totalRecords: 200 },
{ currentPage: 3, pageSize: 20, totalRecords: 100 },
];

for (const props of sampleProps) {
const onPageChange = jest.fn();
const { unmount } = renderWithAuth(
<PaginationWidgetBase {...props} onPageChange={onPageChange} />,
);

const prevButton = await screen.findByLabelText("Previous page");
const nextButton = await screen.findByLabelText("Next page");

expect(prevButton).not.toBeDisabled();
expect(prevButton).toHaveAttribute("aria-disabled", "false");

expect(nextButton).not.toBeDisabled();
expect(nextButton).toHaveAttribute("aria-disabled", "false");

await userEvent.click(prevButton);
await userEvent.click(nextButton);
expect(onPageChange).toHaveBeenCalledTimes(2);
unmount();
}
});
});
Loading