From 76efb27be5b00f6db20af0337dff0054afb39f78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Mon, 30 Sep 2024 15:10:53 +0100 Subject: [PATCH] refactor: Make Virtuoso a HOC at the root of the Search component --- .../Sidebar/Search/NodeSearchResults.test.tsx | 59 ------ .../Sidebar/Search/NodeSearchResults.tsx | 67 ------- .../Sidebar/Search/SearchHeader.test.tsx | 64 +++++++ .../Sidebar/Search/SearchHeader.tsx | 130 ++++++++++++++ .../components/Sidebar/Search/index.test.tsx | 2 +- .../components/Sidebar/Search/index.tsx | 170 ++++++------------ 6 files changed, 247 insertions(+), 245 deletions(-) delete mode 100644 editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/NodeSearchResults.test.tsx delete mode 100644 editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/NodeSearchResults.tsx create mode 100644 editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/SearchHeader.test.tsx create mode 100644 editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/SearchHeader.tsx diff --git a/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/NodeSearchResults.test.tsx b/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/NodeSearchResults.test.tsx deleted file mode 100644 index 0b30a4acad..0000000000 --- a/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/NodeSearchResults.test.tsx +++ /dev/null @@ -1,59 +0,0 @@ -import { useStore } from "pages/FlowEditor/lib/store"; -import React from "react"; -import { setup } from "testUtils"; -import { vi } from "vitest"; -import { axe } from "vitest-axe"; - -import { flow, results } from "./mocks/simple"; -import { NodeSearchResults } from "./NodeSearchResults"; -import { VirtuosoWrapper } from "./testUtils"; - -vi.mock("react-navi", () => ({ - useNavigation: () => ({ - navigate: vi.fn(), - }), -})); - -beforeAll(() => useStore.setState({ flow })); - -it("Displays a warning if no results are returned", () => { - const { getByText, getByRole } = setup( - - - , - ); - expect(getByText("No matches found")).toBeInTheDocument(); - expect(getByRole("list")).toBeEmptyDOMElement(); -}); - -it("Displays the count for a single result", () => { - const { getByText, getByRole, getAllByRole } = setup( - - - , - ); - expect(getByText("1 result:")).toBeInTheDocument(); - expect(getByRole("list")).not.toBeEmptyDOMElement(); - expect(getAllByRole("listitem")).toHaveLength(1); -}); - -it("Displays the count for multiple results", () => { - const { getByText, getByRole, getAllByRole } = setup( - - - , - ); - expect(getByText("2 results:")).toBeInTheDocument(); - expect(getByRole("list")).not.toBeEmptyDOMElement(); - expect(getAllByRole("listitem")).toHaveLength(2); -}); - -it("should not have any accessibility violations on initial load", async () => { - const { container } = setup( - - - , - ); - const axeResults = await axe(container); - expect(axeResults).toHaveNoViolations(); -}); diff --git a/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/NodeSearchResults.tsx b/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/NodeSearchResults.tsx deleted file mode 100644 index 9464e61f90..0000000000 --- a/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/NodeSearchResults.tsx +++ /dev/null @@ -1,67 +0,0 @@ -import Box from "@mui/material/Box"; -import List from "@mui/material/List"; -import ListItem from "@mui/material/ListItem"; -import { styled, useTheme } from "@mui/material/styles"; -import Typography from "@mui/material/Typography"; -import { IndexedNode } from "@opensystemslab/planx-core/types"; -import type { SearchResult, SearchResults } from "hooks/useSearch"; -import React from "react"; -import { Components, Virtuoso } from "react-virtuoso"; - -import { ExternalPortalList } from "./ExternalPortalList"; -import { SearchResultCard } from "./SearchResultCard"; - -export const Root = styled(Box)(({ theme }) => ({ - width: "100%", - gap: theme.spacing(2), - display: "flex", - flexDirection: "column", - height: "100vh", - overflow: "hidden", -})); - -type Data = SearchResult; -type Context = { resultCount: number }; - -const ListComponent = React.forwardRef((props, ref) => ( - -)) as Components["List"]; - -const ListItemComponent = React.forwardRef((props, ref) => ( - -)) as Components["Item"]; - -const HeaderComponent: Components["Header"] = ({ context }) => ( - - {context?.resultCount === 0 && "No matches found"} - {context?.resultCount === 1 && "1 result:"} - {context!.resultCount > 1 && `${context?.resultCount} results:`} - -); - -export const NodeSearchResults: React.FC<{ - results: SearchResults; -}> = ({ results }) => { - const theme = useTheme(); - - return ( - - style={{ - height: "500px", - width: "100%", - gap: theme.spacing(2), - }} - totalCount={results.length} - context={{ - resultCount: results.length, - }} - components={{ - Footer: ExternalPortalList, - List: ListComponent, - Item: ListItemComponent, - Header: HeaderComponent, - }} - itemContent={(index) => } - /> - ); -}; diff --git a/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/SearchHeader.test.tsx b/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/SearchHeader.test.tsx new file mode 100644 index 0000000000..e7e12d6b80 --- /dev/null +++ b/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/SearchHeader.test.tsx @@ -0,0 +1,64 @@ +import { waitFor } from "@testing-library/react"; +import { useStore } from "pages/FlowEditor/lib/store"; +import React from "react"; +import { setup } from "testUtils"; +import { vi } from "vitest"; + +import Search from "."; +import { flow } from "./mocks/simple"; +import { VirtuosoWrapper } from "./testUtils"; + +vi.mock("react-navi", () => ({ + useNavigation: () => ({ + navigate: vi.fn(), + }), +})); + +beforeAll(() => useStore.setState({ flow })); + +it("Displays a warning if no results are returned", async () => { + const { getByLabelText, getByText, getByRole, user } = setup( + + + , + ); + + const searchInput = getByLabelText("Search this flow and internal portals"); + user.type(searchInput, "Timbuktu"); + + await waitFor(() => + expect(getByText("No matches found")).toBeInTheDocument(), + ); + expect(getByRole("list")).toBeEmptyDOMElement(); +}); + +it("Displays the count for a single result", async () => { + const { getByLabelText, getByText, getAllByRole, getByRole, user } = setup( + + + , + ); + + const searchInput = getByLabelText("Search this flow and internal portals"); + user.type(searchInput, "Spain"); + + await waitFor(() => expect(getByText("1 result:")).toBeInTheDocument()); + expect(getByRole("list")).not.toBeEmptyDOMElement(); + expect(getAllByRole("listitem")).toHaveLength(1); +}); + +it("Displays the count for multiple results", async () => { + const { getByText, getByRole, getAllByRole, getByLabelText, user } = setup( + + + , + ); + + const searchInput = getByLabelText("Search this flow and internal portals"); + // Matches "India" and "Indonesia" + user.type(searchInput, "Ind"); + + await waitFor(() => expect(getByText("2 results:")).toBeInTheDocument()); + expect(getByRole("list")).not.toBeEmptyDOMElement(); + expect(getAllByRole("listitem")).toHaveLength(2); +}); diff --git a/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/SearchHeader.tsx b/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/SearchHeader.tsx new file mode 100644 index 0000000000..72115dcec3 --- /dev/null +++ b/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/SearchHeader.tsx @@ -0,0 +1,130 @@ +import Box from "@mui/material/Box"; +import CircularProgress from "@mui/material/CircularProgress"; +import Typography from "@mui/material/Typography"; +import { useFormik } from "formik"; +import { useSearch } from "hooks/useSearch"; +import { debounce } from "lodash"; +import { useStore } from "pages/FlowEditor/lib/store"; +import React, { useEffect, useMemo, useState } from "react"; +import { Components } from "react-virtuoso"; +import ChecklistItem from "ui/shared/ChecklistItem"; +import Input from "ui/shared/Input"; + +import { Context, Data } from "."; +import { DATA_FACETS } from "./facets"; + +const DEBOUNCE_MS = 500; + +interface SearchNodes { + pattern: string; + facets: typeof DATA_FACETS; +} + +/** + * SearchHeader contains the main logic of the search sidebar + * It is nested within the Virtuoso list as a header to allow scrolling to work across the entire sidebar + */ +export const SearchHeader: Components["Header"] = ({ + context, +}) => { + // Get ordered flow of indexed nodes from store + const [orderedFlow, setOrderedFlow] = useStore((state) => [ + state.orderedFlow, + state.setOrderedFlow, + ]); + + useEffect(() => { + if (!orderedFlow) setOrderedFlow(); + }, [orderedFlow, setOrderedFlow]); + + // Set up search input form + const formik = useFormik({ + initialValues: { pattern: "", facets: DATA_FACETS }, + onSubmit: ({ pattern }) => { + debouncedSearch(pattern); + }, + }); + + // Set up spinner UI in search bar + const [isSearching, setIsSearching] = useState(false); + const [lastSearchedTerm, setLastSearchedTerm] = useState(""); + + useEffect(() => { + if (formik.values.pattern !== lastSearchedTerm) { + setIsSearching(true); + } + }, [formik.values.pattern, lastSearchedTerm]); + + // Call custom hook to control searching + const { results, search } = useSearch({ + list: orderedFlow || [], + keys: formik.values.facets, + }); + + const debouncedSearch = useMemo( + () => + debounce((pattern: string) => { + console.debug("Search term: ", pattern); + search(pattern); + setLastSearchedTerm(pattern); + setIsSearching(false); + }, DEBOUNCE_MS), + [search], + ); + + // Update results in parent component (Virtuoso list) + useEffect(() => { + context?.setResults(results); + }, [context, results]); + + return ( + + + Search this flow and internal portals + + + { + formik.setFieldValue("pattern", e.target.value); + formik.handleSubmit(); + }} + inputProps={{ spellCheck: false }} + /> + {isSearching && ( + ({ + position: "absolute", + right: theme.spacing(1.5), + zIndex: 1, + })} + /> + )} + + {}} + variant="compact" + /> + {formik.values.pattern && ( + + {context?.resultCount === 0 && "No matches found"} + {context?.resultCount === 1 && "1 result:"} + {context!.resultCount > 1 && `${context?.resultCount} results:`} + + )} + + ); +}; diff --git a/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/index.test.tsx b/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/index.test.tsx index d10686516c..764f16ad71 100644 --- a/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/index.test.tsx +++ b/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/index.test.tsx @@ -55,7 +55,7 @@ test("entering a search term displays a series of cards", async () => { , ); - expect(queryByRole("list")).not.toBeInTheDocument(); + expect(queryByRole("list")).toBeEmptyDOMElement(); const searchInput = getByLabelText("Search this flow and internal portals"); user.type(searchInput, "ind"); diff --git a/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/index.tsx b/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/index.tsx index 513c02619c..b386eb1c3a 100644 --- a/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/index.tsx +++ b/editor.planx.uk/src/pages/FlowEditor/components/Sidebar/Search/index.tsx @@ -1,128 +1,62 @@ -import Box from "@mui/material/Box"; -import CircularProgress from "@mui/material/CircularProgress"; -import Container from "@mui/material/Container"; -import Typography from "@mui/material/Typography"; +import List from "@mui/material/List"; +import ListItem from "@mui/material/ListItem"; +import { useTheme } from "@mui/material/styles"; import { IndexedNode } from "@opensystemslab/planx-core/types"; -import { useFormik } from "formik"; -import { SearchResult, useSearch } from "hooks/useSearch"; -import { debounce } from "lodash"; -import { useStore } from "pages/FlowEditor/lib/store"; -import React, { useEffect, useMemo, useState } from "react"; -import ChecklistItem from "ui/shared/ChecklistItem"; -import Input from "ui/shared/Input"; - -import { DATA_FACETS } from "./facets"; -import { NodeSearchResults } from "./NodeSearchResults"; - -const DEBOUNCE_MS = 500; - -interface SearchNodes { - pattern: string; - facets: typeof DATA_FACETS; -} - -interface SearchHeaderProps { - onChange: React.Dispatch[]>>; -} - -const SearchHeader: React.FC = ({ onChange }) => { - const [orderedFlow, setOrderedFlow] = useStore((state) => [ - state.orderedFlow, - state.setOrderedFlow, - ]); - const [isSearching, setIsSearching] = useState(false); - const [lastSearchedTerm, setLastSearchedTerm] = useState(""); - - useEffect(() => { - if (!orderedFlow) setOrderedFlow(); - }, [orderedFlow, setOrderedFlow]); - - const formik = useFormik({ - initialValues: { pattern: "", facets: DATA_FACETS }, - onSubmit: ({ pattern }) => { - debouncedSearch(pattern); - }, - }); - - const { results, search } = useSearch({ - list: orderedFlow || [], - keys: formik.values.facets, - }); - - useEffect(() => { - onChange(results); - }, [onChange, results]); - - const debouncedSearch = useMemo( - () => - debounce((pattern: string) => { - console.debug("Search term: ", pattern); - search(pattern); - setLastSearchedTerm(pattern); - setIsSearching(false); - }, DEBOUNCE_MS), - [search], - ); - - useEffect(() => { - if (formik.values.pattern !== lastSearchedTerm) { - setIsSearching(true); - } - }, [formik.values.pattern, lastSearchedTerm]); - - return ( -
- - Search this flow and internal portals - - - { - formik.setFieldValue("pattern", e.target.value); - formik.handleSubmit(); - }} - inputProps={{ spellCheck: false }} - /> - {isSearching && ( - ({ - position: "absolute", - right: theme.spacing(1.5), - zIndex: 1, - })} - /> - )} - - {}} - variant="compact" - /> - - ); +import type { SearchResult, SearchResults } from "hooks/useSearch"; +import React, { useState } from "react"; +import { Components, Virtuoso } from "react-virtuoso"; + +import { ExternalPortalList } from "./ExternalPortalList"; +import { SearchHeader } from "./SearchHeader"; +import { SearchResultCard } from "./SearchResultCard"; + +// Types for Virtuoso +export type Data = SearchResult; +export type Context = { + resultCount: number; + setResults: React.Dispatch>>; }; +/** + * Accessability - Render the Virtuoso list as a HTMLUListElement, not a HTMLDivElement + */ +const ListComponent = React.forwardRef((props, ref) => ( + +)) as Components["List"]; + +/** + * Accessability - Render the Virtuoso item as a HTMLLiElement, not a HTMLDivElement + */ +const ListItemComponent = React.forwardRef((props, ref) => ( + +)) as Components["Item"]; + +/** + * Search uses Virtuoso to generate a virtualised list of search results + * The main logic lives within the useSearch hook and the SearchHeader component + */ const Search: React.FC = () => { const [results, setResults] = useState[]>([]); + const theme = useTheme(); return ( - - - - + + style={{ + marginBottom: theme.spacing(3), + }} + totalCount={results.length} + context={{ + resultCount: results.length, + setResults: setResults, + }} + components={{ + Footer: ExternalPortalList, + List: ListComponent, + Item: ListItemComponent, + Header: SearchHeader, + }} + itemContent={(index) => } + /> ); };