Nov 9, 2023 · Nov 7, 2023 · Nov 7, 2023 · Nov 7, 2023 · Nov 7, 2023 · Nov 7, 2023
diff --git a/site/src/components/PaginationWidget/PageButton.tsx b/site/src/components/PaginationWidget/PageButton.tsx
diff --git a/site/src/components/PaginationWidget/PageButtons.tsx b/site/src/components/PaginationWidget/PageButtons.tsx
 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}`;
 }
diff --git a/site/src/components/PaginationWidget/PaginationNavButton.tsx b/site/src/components/PaginationWidget/PaginationNavButton.tsx
 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;

  // 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);
  }

  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}
    />
  );
 }
diff --git a/site/src/components/PaginationWidget/PaginationWidgetBase.stories.tsx b/site/src/components/PaginationWidget/PaginationWidgetBase.stories.tsx
  title: "components/PaginationWidgetBase",
  component: PaginationWidgetBase,
  args: {
 page: 1,
 limit: 12,
 count: 200,
 currentPage: 1,
 pageSize: 12,
 totalRecords: 200,
  },
 };

 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 },
 };
diff --git a/site/src/components/PaginationWidget/PaginationWidgetBase.test.tsx b/site/src/components/PaginationWidget/PaginationWidgetBase.test.tsx
 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();
    }
  });
 });
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 = <>…</>,
		}: 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}`;
		}
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 aslilacNov 8, 2023 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 MemberAuthor ParkreinerNov 8, 2023 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 aslilacNov 9, 2023 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 aslilacNov 8, 2023 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 MemberAuthor ParkreinerNov 8, 2023• edited Loading 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 aslilacNov 8, 2023 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 MemberAuthor ParkreinerNov 8, 2023 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 },
		};
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();
		}
		});
		});