fix(MachineList): Update pagination behaviour MAASENG-1490 (#4849)

- Error messages are displayed for invalid values - Error messages are cleared on blur and value is reset to current page - Adjusted styling of error message (approved by Amy)
canonical · Apr 3, 2023 · 3cd52be · 3cd52be
1 parent d918639
commit 3cd52be
Show file tree

Hide file tree

Showing 5 changed files with 176 additions and 91 deletions.
diff --git a/...s/views/MachineList/MachineListTable/MachineListPagination/MachineListPagination.test.tsx b/...s/views/MachineList/MachineListTable/MachineListPagination/MachineListPagination.test.tsx
@@ -1,81 +1,137 @@
 import MachineListPagination, { Label } from "./MachineListPagination";
+import type { Props as MachineListPaginationProps } from "./MachineListPagination";
 
-import { render, screen } from "testing/utils";
-
-it("displays pagination if there are machines", () => {
-  render(
-    <MachineListPagination
-      currentPage={1}
-      itemsPerPage={20}
-      machineCount={100}
-      machinesLoading={false}
-      paginate={jest.fn()}
-    />
-  );
-  expect(
-    screen.getByRole("navigation", { name: Label.Pagination })
-  ).toBeInTheDocument();
-});
+import { fireEvent, render, screen, userEvent, waitFor } from "testing/utils";
 
-it("does not display pagination if there are no machines", () => {
-  render(
-    <MachineListPagination
-      currentPage={1}
-      itemsPerPage={20}
-      machineCount={0}
-      machinesLoading={false}
-      paginate={jest.fn()}
-    />
-  );
-  expect(
-    screen.queryByRole("navigation", { name: Label.Pagination })
-  ).not.toBeInTheDocument();
-});
+describe("MachineListPagination", () => {
+  let props: MachineListPaginationProps;
+  beforeEach(() => {
+    props = {
+      currentPage: 1,
+      itemsPerPage: 20,
+      machineCount: 100,
+      machinesLoading: false,
+      paginate: jest.fn(),
+    };
+  });
 
-it("displays pagination while refetching machines", () => {
-  // Set up shared props to make it clear what's changing on rerenders.
-  const props = {
-    currentPage: 1,
-    itemsPerPage: 20,
-    machineCount: 100,
-    machinesLoading: false,
-    paginate: jest.fn(),
-  };
-  const { rerender } = render(<MachineListPagination {...props} />);
-  expect(
-    screen.getByRole("navigation", { name: Label.Pagination })
-  ).toBeInTheDocument();
-  rerender(<MachineListPagination {...props} machinesLoading={true} />);
-  expect(
-    screen.getByRole("navigation", { name: Label.Pagination })
-  ).toBeInTheDocument();
-});
+  it("displays pagination if there are machines", () => {
+    render(<MachineListPagination {...props} />);
+    expect(
+      screen.getByRole("navigation", { name: Label.Pagination })
+    ).toBeInTheDocument();
+  });
+
+  it("does not display pagination if there are no machines", () => {
+    props.machineCount = 0;
+    render(<MachineListPagination {...props} />);
+    expect(
+      screen.queryByRole("navigation", { name: Label.Pagination })
+    ).not.toBeInTheDocument();
+  });
+
+  it("displays pagination while refetching machines", () => {
+    const { rerender } = render(<MachineListPagination {...props} />);
+    expect(
+      screen.getByRole("navigation", { name: Label.Pagination })
+    ).toBeInTheDocument();
+    rerender(<MachineListPagination {...props} machinesLoading={true} />);
+    expect(
+      screen.getByRole("navigation", { name: Label.Pagination })
+    ).toBeInTheDocument();
+  });
+
+  it("hides pagination if there are no refetched machines", () => {
+    const { rerender } = render(<MachineListPagination {...props} />);
+    expect(
+      screen.getByRole("navigation", { name: Label.Pagination })
+    ).toBeInTheDocument();
+    props.machinesLoading = true;
+    rerender(<MachineListPagination {...props} />);
+    expect(
+      screen.getByRole("navigation", { name: Label.Pagination })
+    ).toBeInTheDocument();
+    props.machinesLoading = false;
+    props.machineCount = 0;
+    rerender(<MachineListPagination {...props} />);
+    expect(
+      screen.queryByRole("navigation", { name: Label.Pagination })
+    ).not.toBeInTheDocument();
+  });
+
+  it("calls a function to go to the next page when the 'Next page' button is clicked", async () => {
+    render(<MachineListPagination {...props} />);
+    await userEvent.click(screen.getByRole("button", { name: "Next page" }));
+    expect(props.paginate).toHaveBeenCalledWith(2);
+  });
+
+  it("calls a function to go to the previous page when the 'Previous page' button is clicked", async () => {
+    props.currentPage = 2;
+
+    render(<MachineListPagination {...props} />);
+    await userEvent.click(
+      screen.getByRole("button", { name: "Previous page" })
+    );
+    expect(props.paginate).toHaveBeenCalledWith(1);
+  });
+
+  it("takes an input for page number and calls a function to paginate if the number is valid", async () => {
+    render(<MachineListPagination {...props} />);
+
+    const pageInput = screen.getByRole("spinbutton", { name: "page number" });
+
+    // Using userEvent to clear this first doesn't work, so we have to use fireEvent instead.
+    fireEvent.change(pageInput, { target: { value: "4" } });
+    await userEvent.click(pageInput);
+    await userEvent.keyboard("{Enter}");
+
+    await waitFor(() => {
+      expect(props.paginate).toHaveBeenCalledWith(4);
+    });
+  });
+
+  it("displays an error if no value is present in the page number input", async () => {
+    render(<MachineListPagination {...props} />);
+
+    const pageInput = screen.getByRole("spinbutton", { name: "page number" });
+
+    await userEvent.clear(pageInput);
+    expect(screen.getByText(/Enter a page number/i)).toBeInTheDocument();
+  });
+
+  it("displays an error if an invalid page number is entered", async () => {
+    render(<MachineListPagination {...props} />);
+
+    const pageInput = screen.getByRole("spinbutton", { name: "page number" });
+
+    // Using userEvent to clear this first doesn't work, so we have to use fireEvent instead.
+    fireEvent.change(pageInput, { target: { value: "69" } });
+    await waitFor(() => {
+      expect(
+        screen.getByText(/"69" is not a valid page number/i)
+      ).toBeInTheDocument();
+    });
+  });
+
+  it("reverts the value to the current page number and hides error messages if the input is blurred", async () => {
+    render(<MachineListPagination {...props} />);
+
+    const pageInput = screen.getByRole("spinbutton", { name: "page number" });
+
+    fireEvent.change(pageInput, { target: { value: "69" } });
+
+    await waitFor(() => {
+      expect(
+        screen.getByText(/"69" is not a valid page number/i)
+      ).toBeInTheDocument();
+    });
+
+    fireEvent.blur(pageInput);
+
+    expect(
+      screen.queryByText(/"69" is not a valid page number/i)
+    ).not.toBeInTheDocument();
 
-it("hides pagination if there are no refetched machines", () => {
-  // Set up shared props to make it clear what's changing on rerenders.
-  const props = {
-    currentPage: 1,
-    itemsPerPage: 20,
-    machineCount: 100,
-    machinesLoading: false,
-    paginate: jest.fn(),
-  };
-  const { rerender } = render(<MachineListPagination {...props} />);
-  expect(
-    screen.getByRole("navigation", { name: Label.Pagination })
-  ).toBeInTheDocument();
-  rerender(<MachineListPagination {...props} machinesLoading={true} />);
-  expect(
-    screen.getByRole("navigation", { name: Label.Pagination })
-  ).toBeInTheDocument();
-  rerender(
-    <MachineListPagination
-      {...props}
-      machineCount={0}
-      machinesLoading={false}
-    />
-  );
-  expect(
-    screen.queryByRole("navigation", { name: Label.Pagination })
-  ).not.toBeInTheDocument();
+    expect(pageInput).toHaveValue(1);
+  });
 });
diff --git a/...chines/views/MachineList/MachineListTable/MachineListPagination/MachineListPagination.tsx b/...chines/views/MachineList/MachineListTable/MachineListPagination/MachineListPagination.tsx
@@ -1,4 +1,4 @@
-import { useRef, useState, useEffect } from "react";
+import { useState, useRef, useEffect } from "react";
 
 import type {
   PaginationProps,
@@ -16,7 +16,7 @@ export enum Label {
 
 export const DEFAULT_DEBOUNCE_INTERVAL = 500;
 
-type Props = PropsWithSpread<
+export type Props = PropsWithSpread<
   {
     currentPage: PaginationProps["currentPage"];
     itemsPerPage: PaginationProps["itemsPerPage"];
@@ -33,7 +33,10 @@ const MachineListPagination = ({
   ...props
 }: Props): JSX.Element | null => {
   const intervalRef = useRef<NodeJS.Timeout | null>(null);
-  const [pageNumber, setPageNumber] = useState(props.currentPage);
+  const [pageNumber, setPageNumber] = useState<number | undefined>(
+    props.currentPage
+  );
+  const [error, setError] = useState("");
 
   // Clear the timeout when the component is unmounted.
   useEffect(() => {
@@ -64,20 +67,36 @@ const MachineListPagination = ({
         <Input
           aria-label="page number"
           className="p-pagination__input"
+          error={error}
+          onBlur={() => {
+            setPageNumber(props.currentPage);
+            setError("");
+          }}
           onChange={(e) => {
-            setPageNumber(e.target.valueAsNumber);
-            if (intervalRef.current) {
-              clearTimeout(intervalRef.current);
-            }
-            intervalRef.current = setTimeout(() => {
-              if (
-                e.target.valueAsNumber > 0 &&
-                e.target.valueAsNumber <= totalPages
-              ) {
-                props.paginate(e.target.valueAsNumber);
+            if (e.target.value) {
+              setPageNumber(e.target.valueAsNumber);
+              if (intervalRef.current) {
+                clearTimeout(intervalRef.current);
               }
-            }, DEFAULT_DEBOUNCE_INTERVAL);
+              intervalRef.current = setTimeout(() => {
+                if (
+                  e.target.valueAsNumber > totalPages ||
+                  e.target.valueAsNumber < 1
+                ) {
+                  setError(
+                    `"${e.target.valueAsNumber}" is not a valid page number.`
+                  );
+                } else {
+                  setError("");
+                  props.paginate(e.target.valueAsNumber);
+                }
+              }, DEFAULT_DEBOUNCE_INTERVAL);
+            } else {
+              setPageNumber(undefined);
+              setError("Enter a page number.");
+            }
           }}
+          required
           type="number"
           value={pageNumber}
         />{" "}

diff --git a/src/app/machines/views/MachineList/MachineListTable/MachineListPagination/_index.scss b/src/app/machines/views/MachineList/MachineListTable/MachineListPagination/_index.scss
@@ -21,6 +21,12 @@
       text-align: center;
     }
 
+    .p-form-validation__message {
+      position: absolute;
+      margin-top: $spv--small;
+      margin-left: $spv--small;
+    }
+
     // required to get rid of buttons/arrows while keeping number-only input
     input::-webkit-outer-spin-button,
     input::-webkit-inner-spin-button {

diff --git a/src/app/machines/views/MachineList/MachineListTable/MachineListTable.tsx b/src/app/machines/views/MachineList/MachineListTable/MachineListTable.tsx
@@ -832,7 +832,7 @@ export const MachineListTable = ({
   return (
     <>
       {machineCount ? (
-        <div className="u-flex--between u-flex--align-end u-flex--wrap">
+        <div className="u-flex--between u-flex--align-baseline u-flex--wrap">
           <hr />
           <MachineListDisplayCount
             currentPage={currentPage}
@@ -855,7 +855,7 @@ export const MachineListTable = ({
               />
             ) : null}
           </span>
-          <hr />
+          <hr className="machine-list__divider" />
         </div>
       ) : null}
       <MainTable

diff --git a/src/app/machines/views/MachineList/_index.scss b/src/app/machines/views/MachineList/_index.scss
@@ -111,6 +111,10 @@
     justify-content: flex-end;
   }
 
+  .machine-list__divider {
+    margin-top: $spv--x-large;
+  }
+
   .p-tooltip__message .p-list::after {
     white-space: normal;
   }