From 33bea3e102967e756044773ae390eb5caf167ebe Mon Sep 17 00:00:00 2001 From: Doug Schrashun Date: Wed, 15 Jan 2025 10:29:52 -0500 Subject: [PATCH 1/3] set pagination values correctly --- .../components/search/SearchPagination.tsx | 42 ++++++++++++++----- .../search/SearchPaginationFetch.tsx | 2 +- .../src/components/search/SearchSortBy.tsx | 10 +---- 3 files changed, 33 insertions(+), 21 deletions(-) diff --git a/frontend/src/components/search/SearchPagination.tsx b/frontend/src/components/search/SearchPagination.tsx index 5d37f75ac..c07b158e7 100644 --- a/frontend/src/components/search/SearchPagination.tsx +++ b/frontend/src/components/search/SearchPagination.tsx @@ -3,7 +3,7 @@ import { QueryContext } from "src/app/[locale]/search/QueryProvider"; import { useSearchParamUpdater } from "src/hooks/useSearchParamUpdater"; -import { useContext } from "react"; +import { useContext, useEffect } from "react"; import { Pagination } from "@trussworks/react-uswds"; export enum PaginationPosition { @@ -14,7 +14,7 @@ export enum PaginationPosition { interface SearchPaginationProps { page: number; query: string | null | undefined; - total?: number | null; + totalPages?: number | null; scroll?: boolean; totalResults?: string; loading?: boolean; @@ -22,32 +22,52 @@ interface SearchPaginationProps { const MAX_SLOTS = 7; +// in addition to handling client side page navigation, this client component handles setting client state for: +// - total pages of search results +// - total number of search results export default function SearchPagination({ page, query, - total = null, + totalPages = null, scroll = false, totalResults = "", loading = false, }: SearchPaginationProps) { const { updateQueryParams } = useSearchParamUpdater(); - const { updateTotalPages, updateTotalResults } = useContext(QueryContext); - const { totalPages } = useContext(QueryContext); - // Shows total pages from the query context before it is re-fetched from the API. - const pages = total || Number(totalPages); + const { + updateTotalPages, + updateTotalResults, + totalPages: totalPagesFromQuery, + } = useContext(QueryContext); + + // will re-run on each fetch, as we switch from the Suspended version of the component to the live one + useEffect(() => { + if (!loading) { + updateTotalPages(String(totalPages)); + } + }, [updateTotalPages, totalPages, loading]); + + useEffect(() => { + if (!loading) { + updateTotalResults(String(totalResults)); + } + }, [updateTotalResults, totalResults, loading]); const updatePage = (page: number) => { - updateTotalPages(String(total)); - updateTotalResults(totalResults); updateQueryParams(String(page), "page", query, scroll); }; + // Shows total pages from the query context before it is re-fetched from the API. + // This is only used for display due to race conditions, otherwise totalPages prop + // is the source of truth + const pageCount = totalPages || Number(totalPagesFromQuery); + return (
- {totalResults !== "0" && pages > 0 && ( + {totalResults !== "0" && pageCount > 0 && ( updatePage(page + 1)} diff --git a/frontend/src/components/search/SearchPaginationFetch.tsx b/frontend/src/components/search/SearchPaginationFetch.tsx index 334f91c32..169b05f92 100644 --- a/frontend/src/components/search/SearchPaginationFetch.tsx +++ b/frontend/src/components/search/SearchPaginationFetch.tsx @@ -26,7 +26,7 @@ export default async function SearchPaginationFetch({ return ( <> ) => { const newValue = event.target.value; - updateTotalResults(totalResults); updateQueryParams(newValue, "sortby", queryTerm); }; From 25fdf7a7ecb580bc72d48bcbd0e8cf8361735d94 Mon Sep 17 00:00:00 2001 From: Doug Schrashun Date: Wed, 15 Jan 2025 10:40:16 -0500 Subject: [PATCH 2/3] tests and fixes --- .../src/app/[locale]/search/QueryProvider.tsx | 2 +- .../components/search/SearchPagination.tsx | 1 + .../components/search/SearchResultsHeader.tsx | 8 +- .../src/components/search/SearchSortBy.tsx | 13 +- .../search/SearchPagination.test.tsx | 234 +++++++++++++++++- .../components/search/SearchSortBy.test.tsx | 30 +-- 6 files changed, 239 insertions(+), 49 deletions(-) diff --git a/frontend/src/app/[locale]/search/QueryProvider.tsx b/frontend/src/app/[locale]/search/QueryProvider.tsx index 1efdff70c..c85607122 100644 --- a/frontend/src/app/[locale]/search/QueryProvider.tsx +++ b/frontend/src/app/[locale]/search/QueryProvider.tsx @@ -3,7 +3,7 @@ import { useSearchParams } from "next/navigation"; import { createContext, useCallback, useMemo, useState } from "react"; -interface QueryContextParams { +export interface QueryContextParams { queryTerm: string | null | undefined; updateQueryTerm: (term: string) => void; totalPages: string | null | undefined; diff --git a/frontend/src/components/search/SearchPagination.tsx b/frontend/src/components/search/SearchPagination.tsx index c07b158e7..4c5e45637 100644 --- a/frontend/src/components/search/SearchPagination.tsx +++ b/frontend/src/components/search/SearchPagination.tsx @@ -66,6 +66,7 @@ export default function SearchPagination({
{totalResults !== "0" && pageCount > 0 && (
- +
); diff --git a/frontend/src/components/search/SearchSortBy.tsx b/frontend/src/components/search/SearchSortBy.tsx index 2b6322e10..2c1df3fcb 100644 --- a/frontend/src/components/search/SearchSortBy.tsx +++ b/frontend/src/components/search/SearchSortBy.tsx @@ -4,12 +4,12 @@ import { useSearchParamUpdater } from "src/hooks/useSearchParamUpdater"; import { SortOption } from "src/types/search/searchRequestTypes"; import { useTranslations } from "next-intl"; +import { useCallback } from "react"; import { Select } from "@trussworks/react-uswds"; interface SearchSortByProps { queryTerm: string | null | undefined; sortby: string | null; - totalResults: string; } export default function SearchSortBy({ queryTerm, sortby }: SearchSortByProps) { @@ -45,10 +45,13 @@ export default function SearchSortBy({ queryTerm, sortby }: SearchSortByProps) { }, ]; - const handleChange = (event: React.ChangeEvent) => { - const newValue = event.target.value; - updateQueryParams(newValue, "sortby", queryTerm); - }; + const handleChange = useCallback( + (event: React.ChangeEvent) => { + const newValue = event.target.value; + updateQueryParams(newValue, "sortby", queryTerm); + }, + [queryTerm, updateQueryParams], + ); return (
diff --git a/frontend/tests/components/search/SearchPagination.test.tsx b/frontend/tests/components/search/SearchPagination.test.tsx index 8eb2477db..3f9963626 100644 --- a/frontend/tests/components/search/SearchPagination.test.tsx +++ b/frontend/tests/components/search/SearchPagination.test.tsx @@ -3,29 +3,57 @@ import "@testing-library/jest-dom/extend-expect"; import { render, screen } from "@testing-library/react"; import { axe } from "jest-axe"; +import { QueryContext } from "src/app/[locale]/search/QueryProvider"; import React from "react"; import SearchPagination from "src/components/search/SearchPagination"; +let fakeTotalPages = "1"; +const mockUpdateTotalPages = jest.fn(); +const mockUpdateTotalResults = jest.fn(); +const mockUpdateQueryParams = jest.fn(); + // Mock the useSearchParamUpdater hook jest.mock("src/hooks/useSearchParamUpdater", () => ({ useSearchParamUpdater: () => ({ - updateQueryParams: jest.fn(), + updateQueryParams: mockUpdateQueryParams, }), })); +const FakeQueryProvider = ({ children }: { children: React.ReactNode }) => { + const contextValue = { + updateTotalPages: mockUpdateTotalPages, + updateTotalResults: mockUpdateTotalResults, + totalPages: fakeTotalPages, + queryTerm: "", + // eslint-disable-next-line + updateQueryTerm: () => {}, + totalResults: "", + }; + return ( + + {children} + + ); +}; + beforeEach(() => { jest.clearAllMocks(); }); describe("SearchPagination", () => { beforeEach(() => { + fakeTotalPages = "1"; jest.clearAllMocks(); }); it("should not have basic accessibility issues", async () => { - const { container } = render(); + const { container } = render( + + + , + ); const results = await axe(container, { rules: { @@ -37,33 +65,217 @@ describe("SearchPagination", () => { expect(results).toHaveNoViolations(); }); it("Renders Pagination component when pages > 0", () => { - render(); + render( + + + , + ); expect(screen.getByRole("navigation")).toBeInTheDocument(); }); it("Does not render Pagination component when pages <= 0", () => { - render(); + fakeTotalPages = "0"; + render( + + + , + ); expect(screen.queryByRole("navigation")).not.toBeInTheDocument(); }); + it("renders disabled Pagination component when loading", () => { + render( + + + , + ); + + // some problem with the testing reading the applied styles, so can't figure out + // how to assert that things are actually disabled here + expect(screen.queryByRole("navigation")).toHaveAttribute("aria-disabled"); + }); + it("Renders Pagination component when totalResults > 0", () => { render( - , + + + , ); expect(screen.getByRole("navigation")).toBeInTheDocument(); }); it("Does not render Pagination component when total results is 0", () => { render( - , + + + , ); expect(screen.queryByRole("navigation")).not.toBeInTheDocument(); }); + it("updates client state for total pages based on total pages prop", () => { + const { rerender } = render( + + + , + ); + + expect(mockUpdateTotalPages).toHaveBeenCalledWith("11"); + rerender( + + + , + ); + expect(mockUpdateTotalPages).toHaveBeenCalledWith("12"); + }); + + it("does not update client state for total pages while loading", () => { + const { rerender } = render( + + + , + ); + + expect(mockUpdateTotalPages).not.toHaveBeenCalled(); + rerender( + + + , + ); + expect(mockUpdateTotalPages).not.toHaveBeenCalled(); + }); + it("updates client state for total results based on total results prop", () => { + const { rerender } = render( + + + , + ); + + expect(mockUpdateTotalResults).toHaveBeenCalledWith("11"); + rerender( + + + , + ); + expect(mockUpdateTotalResults).toHaveBeenCalledWith("12"); + }); + + it("does not update client state for total results while loading", () => { + const { rerender } = render( + + + , + ); + + expect(mockUpdateTotalResults).not.toHaveBeenCalled(); + rerender( + + + , + ); + expect(mockUpdateTotalResults).not.toHaveBeenCalled(); + }); + + it("updates page state on clicks as expected", () => { + const { rerender } = render( + + + , + ); + const pageNumberButton = screen.queryByLabelText("Page 2"); + pageNumberButton?.click(); + expect(mockUpdateQueryParams).toHaveBeenCalledTimes(1); + expect(mockUpdateQueryParams).toHaveBeenCalledWith( + "2", + "page", + "test", + false, + ); + + rerender( + + + , + ); + const previousButton = screen.queryByLabelText("Previous page"); + previousButton?.click(); + + expect(mockUpdateQueryParams).toHaveBeenCalledTimes(2); + expect(mockUpdateQueryParams).toHaveBeenCalledWith( + "1", + "page", + "test", + false, + ); + }); }); diff --git a/frontend/tests/components/search/SearchSortBy.test.tsx b/frontend/tests/components/search/SearchSortBy.test.tsx index 7aef22202..fce7c3aa6 100644 --- a/frontend/tests/components/search/SearchSortBy.test.tsx +++ b/frontend/tests/components/search/SearchSortBy.test.tsx @@ -1,23 +1,11 @@ import { fireEvent } from "@testing-library/react"; import { axe } from "jest-axe"; -import { identity } from "lodash"; -import { QueryContext } from "src/app/[locale]/search/QueryProvider"; import { useTranslationsMock } from "src/utils/testing/intlMocks"; import { render, screen } from "tests/react-utils"; import SearchSortBy from "src/components/search/SearchSortBy"; const updateQueryParamsMock = jest.fn(); -const updateTotalResultsMock = jest.fn(); - -const fakeContext = { - queryTerm: "", - updateQueryTerm: identity, - totalPages: "1", - updateTotalPages: identity, - totalResults: "1", - updateTotalResults: updateTotalResultsMock, -}; // Mock the useSearchParamUpdater hook jest.mock("src/hooks/useSearchParamUpdater", () => ({ @@ -34,11 +22,7 @@ jest.mock("next-intl", () => ({ describe("SearchSortBy", () => { it("should not have basic accessibility issues", async () => { const { container } = render( - , + , ); const results = await axe(container); @@ -46,7 +30,7 @@ describe("SearchSortBy", () => { }); it("renders correctly with initial query params", () => { - render(); + render(); const defaultOption = screen.getByRole("option", { selected: true, @@ -58,7 +42,7 @@ describe("SearchSortBy", () => { }); it("updates sort option and on change", () => { - render(); + render(); let selectedOption = screen.getByRole("option", { selected: true, @@ -82,11 +66,7 @@ describe("SearchSortBy", () => { }); it("calls expected search update functions on change", () => { - render( - - - , - ); + render(); fireEvent.change(screen.getByLabelText("sortBy.label"), { target: { value: "opportunityTitleDesc" }, @@ -97,7 +77,5 @@ describe("SearchSortBy", () => { "sortby", "test", ); - - expect(updateTotalResultsMock).toHaveBeenCalledWith("10"); }); }); From 621edc4090943c65bc774d8350dc12b501c2a6a7 Mon Sep 17 00:00:00 2001 From: Doug Schrashun Date: Fri, 17 Jan 2025 12:46:04 -0500 Subject: [PATCH 3/3] remove unused export --- frontend/src/app/[locale]/search/QueryProvider.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/app/[locale]/search/QueryProvider.tsx b/frontend/src/app/[locale]/search/QueryProvider.tsx index c85607122..1efdff70c 100644 --- a/frontend/src/app/[locale]/search/QueryProvider.tsx +++ b/frontend/src/app/[locale]/search/QueryProvider.tsx @@ -3,7 +3,7 @@ import { useSearchParams } from "next/navigation"; import { createContext, useCallback, useMemo, useState } from "react"; -export interface QueryContextParams { +interface QueryContextParams { queryTerm: string | null | undefined; updateQueryTerm: (term: string) => void; totalPages: string | null | undefined;