Skip to content
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

[Issue #2732] clear search input when clicking search nav link #2756

Merged
merged 5 commits into from
Nov 14, 2024
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
1 change: 1 addition & 0 deletions frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"node": ">=20.0.0"
},
"scripts": {
"all-checks": "npm run test && npm run lint && npm run ts:check && npm run build",
Copy link
Contributor

Choose a reason for hiding this comment

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

love this!! kudos!

"build": "next build",
"dev": "next dev",
"debug": "NODE_OPTIONS='--inspect' next dev",
Expand Down
97 changes: 62 additions & 35 deletions frontend/src/components/Header.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
"use client";

import { useFeatureFlags } from "src/hooks/useFeatureFlags";
import { assetPath } from "src/utils/assetPath";

import { useTranslations } from "next-intl";
import Link from "next/link";
import { useEffect, useRef, useState } from "react";
import { usePathname } from "next/navigation";
import { useCallback, useMemo, useState } from "react";
import {
GovBanner,
NavMenuButton,
Expand All @@ -14,47 +14,75 @@ import {
Header as USWDSHeader,
} from "@trussworks/react-uswds";

type PrimaryLinks = {
i18nKey: string;
href: string;
}[];
type PrimaryLink = {
text?: string;
href?: string;
};

type Props = {
logoPath?: string;
locale?: string;
};

const Header = ({ logoPath, locale }: Props) => {
const NavLinks = ({
mobileExpanded,
onToggleMobileNav,
}: {
mobileExpanded: boolean;
onToggleMobileNav: () => unknown;
}) => {
const t = useTranslations("Header");
const [isMobileNavExpanded, setIsMobileNavExpanded] = useState(false);
const handleMobileNavToggle = () => {
setIsMobileNavExpanded(!isMobileNavExpanded);
};
const path = usePathname();

const primaryLinksRef = useRef<PrimaryLinks>([]);
const { featureFlagsManager } = useFeatureFlags();
const getSearchLink = useCallback(
(onSearch: boolean) => {
return {
text: t("nav_link_search"),
href: onSearch ? "/search?refresh=true" : "/search",
};
},
[t],
);

useEffect(() => {
primaryLinksRef.current = [
{ i18nKey: t("nav_link_home"), href: "/" },
{ i18nKey: t("nav_link_process"), href: "/process" },
{ i18nKey: t("nav_link_research"), href: "/research" },
{ i18nKey: t("nav_link_subscribe"), href: "/subscribe" },
const navLinkList = useMemo(() => {
return [
{ text: t("nav_link_home"), href: "/" },
getSearchLink(path.includes("/search")),
{ text: t("nav_link_process"), href: "/process" },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't get a chance to review this, but this could be simplified by just including an array of links instead of an array of objects that is then mapped to links. Something for future clean-up.

{ text: t("nav_link_research"), href: "/research" },
{ text: t("nav_link_subscribe"), href: "/subscribe" },
];
const searchNavLink = {
i18nKey: t("nav_link_search"),
href: "/search",
};
if (featureFlagsManager.isFeatureEnabled("showSearchV0")) {
primaryLinksRef.current.splice(1, 0, searchNavLink);
}
}, [featureFlagsManager, t]);
}, [t, path, getSearchLink]);

const navItems = useMemo(() => {
return navLinkList.map((link: PrimaryLink) => {
if (!link.text || !link.href) {
return <></>;
}
return (
<Link href={link.href} key={link.href}>
{link.text}
</Link>
);
});
}, [navLinkList]);

return (
<PrimaryNav
items={navItems}
mobileExpanded={mobileExpanded}
onToggleMobileNav={onToggleMobileNav}
></PrimaryNav>
);
};

const navItems = primaryLinksRef.current.map((link) => (
<Link href={link.href} key={link.href}>
{link.i18nKey}
</Link>
));
const Header = ({ logoPath, locale }: Props) => {
const t = useTranslations("Header");
const [isMobileNavExpanded, setIsMobileNavExpanded] =
useState<boolean>(false);
const handleMobileNavToggle = () => {
setIsMobileNavExpanded(!isMobileNavExpanded);
};
const language = locale && locale.match("/^es/") ? "spanish" : "english";

return (
Expand Down Expand Up @@ -85,11 +113,10 @@ const Header = ({ logoPath, locale }: Props) => {
label={t("nav_menu_toggle")}
/>
</div>
<PrimaryNav
items={navItems}
<NavLinks
mobileExpanded={isMobileNavExpanded}
onToggleMobileNav={handleMobileNavToggle}
></PrimaryNav>
/>
</div>
</USWDSHeader>
</>
Expand Down
23 changes: 20 additions & 3 deletions frontend/src/components/search/SearchBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,38 @@ import { QueryContext } from "src/app/[locale]/search/QueryProvider";
import { useSearchParamUpdater } from "src/hooks/useSearchParamUpdater";

import { useTranslations } from "next-intl";
import { useContext } from "react";
import { useContext, useEffect, useRef } from "react";
import { Icon } from "@trussworks/react-uswds";

interface SearchBarProps {
query: string | null | undefined;
}

export default function SearchBar({ query }: SearchBarProps) {
const inputRef = useRef<HTMLInputElement>(null);
const { queryTerm, updateQueryTerm } = useContext(QueryContext);
const { updateQueryParams } = useSearchParamUpdater();
const { updateQueryParams, searchParams } = useSearchParamUpdater();
const t = useTranslations("Search");

const handleSubmit = () => {
updateQueryParams("", "query", queryTerm, false);
};

const t = useTranslations("Search");
// if we have "refresh=true" query param, clear the input
// this supports the expected refresh of the input if the user clicks the search link while on the search page
useEffect(() => {
if (searchParams.get("refresh") && inputRef.current) {
updateQueryTerm("");
inputRef.current.value = "";
}
}, [searchParams, updateQueryTerm]);

// removes the "refresh" param once a user has dirtied the input
useEffect(() => {
if (searchParams.get("refresh") && inputRef.current?.value) {
updateQueryParams("", "refresh");
}
}, [searchParams, updateQueryParams]);

return (
<div className="margin-top-5 margin-bottom-2">
Expand All @@ -36,6 +52,7 @@ export default function SearchBar({ query }: SearchBarProps) {
</label>
<div className="usa-search usa-search--big" role="search">
<input
ref={inputRef}
Copy link
Contributor

Choose a reason for hiding this comment

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

Non blocking: Is it worth just adding either useState or possibly the newer server action state management for this form as opposed to using refs and essentially handling setting state manually?

className="usa-input maxw-none"
id="query"
type="search"
Expand Down
4 changes: 3 additions & 1 deletion frontend/src/hooks/useSearchParamUpdater.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,15 @@ export function useSearchParamUpdater() {
const router = useRouter();
const params = new URLSearchParams(searchParams);

// note that providing an empty string as `queryParamValue` will remove the param
const updateQueryParams = (
// The parameter value that is not the query term. Query term is treated
// separately because updates to it are captured, ie if a user updates the
// search term and then clicks a facet, the updated term is used.
queryParamValue: string | Set<string>,
// Key of the parameter.
key: string,
queryTerm: string | null | undefined,
queryTerm?: string | null,
// Determines whether the state update scrolls the user to the top. This
// is useful for components that are expected to be "under the fold."
scroll = false,
Expand Down Expand Up @@ -49,6 +50,7 @@ export function useSearchParamUpdater() {
};

return {
searchParams,
updateQueryParams,
};
}
Expand Down
44 changes: 34 additions & 10 deletions frontend/tests/components/Header.test.tsx
Original file line number Diff line number Diff line change
@@ -1,22 +1,29 @@
import userEvent from "@testing-library/user-event";
import { render, screen } from "tests/react-utils";

import { ReadonlyURLSearchParams } from "next/navigation";

import Header from "src/components/Header";

const props = {
logoPath: "/img/logo.svg",
primaryLinks: [
{
i18nKey: "nav_link_home",
href: "/",
},
{
i18nKey: "nav_link_health",
href: "/health",
},
],
locale: "en",
};

let mockedPath = "/fakepath";

const getMockedPath = () => mockedPath;

jest.mock("src/hooks/useSearchParamUpdater", () => ({
useSearchParamUpdater: () => ({
searchParams: new ReadonlyURLSearchParams(),
}),
}));

jest.mock("next/navigation", () => ({
usePathname: () => getMockedPath(),
}));

describe("Header", () => {
it("toggles the mobile nav menu", async () => {
render(<Header {...props} />);
Expand Down Expand Up @@ -50,4 +57,21 @@ describe("Header", () => {

expect(govBanner).toHaveAttribute("aria-expanded", "true");
});

it("displays a search link without refresh param if not currently on search page", () => {
render(<Header />);

const searchLink = screen.getByRole("link", { name: "Search" });
expect(searchLink).toBeInTheDocument();
expect(searchLink).toHaveAttribute("href", "/search");
});

it("displays a search link with refresh param if currently on search page", () => {
mockedPath = "/search";
render(<Header />);

const searchLink = screen.getByRole("link", { name: "Search" });
expect(searchLink).toBeInTheDocument();
expect(searchLink).toHaveAttribute("href", "/search?refresh=true");
});
});
2 changes: 2 additions & 0 deletions frontend/tests/components/search/SearchBar.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { axe } from "jest-axe";
import QueryProvider from "src/app/[locale]/search/QueryProvider";
import { render, screen } from "tests/react-utils";

import { ReadonlyURLSearchParams } from "next/navigation";
import React from "react";

import SearchBar from "src/components/search/SearchBar";
Expand All @@ -15,6 +16,7 @@ const mockUpdateQueryParams = jest.fn();
jest.mock("src/hooks/useSearchParamUpdater", () => ({
useSearchParamUpdater: () => ({
updateQueryParams: mockUpdateQueryParams,
searchParams: new ReadonlyURLSearchParams(),
}),
}));

Expand Down
8 changes: 8 additions & 0 deletions frontend/tests/pages/search/page.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import Search from "src/app/[locale]/search/page";
import { SEARCH_NO_STATUS_VALUE } from "src/constants/search";
import { useTranslationsMock } from "src/utils/testing/intlMocks";

import { ReadonlyURLSearchParams } from "next/navigation";

// test without feature flag functionality
jest.mock("src/hoc/search/withFeatureFlag", () =>
jest.fn((Component: React.Component) => Component),
Expand All @@ -18,6 +20,12 @@ jest.mock("next-intl", () => ({
useTranslations: () => useTranslationsMock(),
}));

jest.mock("src/hooks/useSearchParamUpdater", () => ({
useSearchParamUpdater: () => ({
searchParams: new ReadonlyURLSearchParams(),
}),
}));

// // currently, with Suspense mocked out below to always show fallback content,
// // the components making the fetch calls are never being rendered so we do not need to mock them out
// // uncomment this if we figure out a way to properly test the underlying async components
Expand Down
Loading