Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

Add suspense boundary around search page results #101

Merged
merged 37 commits into from
Aug 6, 2024
Merged

Conversation

acouch
Copy link
Member

@acouch acouch commented Jun 24, 2024

Summary

Fixes #59

Time to review: 30 mins

Changes proposed

This makes the search page static and adds a suspense boundary for the data being fetched by the server. The data comes from the API and is called from 3 components:

This also simplifies the state model by pushing state changes directly to the browser query params and rerendering the changed items. This makes things a lot simpler and thus a lot of state management code is removed and there results list is no longer wrapped in a form and passing form refs between components. This is the recommended approach by next: https://nextjs.org/learn/dashboard-app/adding-search-and-pagination

There are several items that needed to be shared among the client components: the query, total results count, and total pages. These are wrapped in a <QueryProvider /> that updates the state of these items. This was added so that if someone enters a query in the text box and the clicks a filter their query is not lost, so that the "N Opportunities" text doesn't need to be rerendered when paging or sorting, and so that the pager stays the same length when paging or sorting.

The data is fetched a couple of times in a duplicative fashion, however this follows NextJS best practice since the requests are cached.

The pager has been updated to reload only when there is a change in the page length. Because of an issue with the way the pager renders, it is unavailable while data is being updated:

image

This is because the Truss React component switches between a link and a button as it renders and there isn't an option to supply query arguments, so if a user where to click it they would lose the query params.

Overall this puts us on nice footing for the upcoming work using NextJS best practice.

@acouch acouch changed the title Add suspense boundary around search page Add suspense boundary around search page results Jun 26, 2024
Copy link

github-actions bot commented Jul 5, 2024

Coverage report for ./frontend

St.
Category Percentage Covered / Total
🟢 Statements
89.91% (+6.19% 🔼)
695/773
🟡 Branches
71.18% (+4.19% 🔼)
205/288
🟢 Functions
86.31% (+9.24% 🔼)
145/168
🟢 Lines
90.59% (+6.77% 🔼)
655/723
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / QueryProvider.tsx
94.44% 100% 80% 94.12%
🟡
... / SearchPagination.tsx
66.67% 71.43% 33.33% 66.67%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / SearchFilterAccordion.tsx
70.27% (-6.65% 🔻)
25% (-25% 🔻)
54.55% (+4.55% 🔼)
70.59% (-4.41% 🔻)
🟢
... / SearchFilterSection.tsx
84% (-3.1% 🔻)
55.56% (-30.16% 🔻)
66.67% (-6.06% 🔻)
83.33% (-3.33% 🔻)
🟢
... / useSearchParamUpdater.ts
96% (-4% 🔻)
71.43% (-28.57% 🔻)
100%
96% (-4% 🔻)
🟢
... / SearchBar.tsx
83.33% (-16.67% 🔻)
33.33% (-66.67% 🔻)
75% (-25% 🔻)
90.91% (-9.09% 🔻)
🟢
... / SearchResultsHeader.tsx
88.89% (-11.11% 🔻)
60% (-40% 🔻)
100% 100%

Test suite run success

154 tests passing in 47 suites.

Report generated by 🧪jest coverage report action from 78da679

@acouch acouch marked this pull request as ready for review July 5, 2024 23:01
@acouch acouch requested a review from andycochran as a code owner July 5, 2024 23:01
Comment on lines 74 to 75
// Pass 'true' to handle the special case of splitting 'funding-instrument-'after the second dash
return this.getValuesByKeyPrefix("funding-instrument-", true);
Copy link
Member

Choose a reason for hiding this comment

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

Should funding-instrument- be funding_instrument- (with an underscore) instead? I imagine this'll come up again. We might want to establish a standard for how we format our query parameters. Tho I'm not sure if there are downsides to underscores.

Copy link
Member

@andycochran andycochran Jul 8, 2024

Choose a reason for hiding this comment

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

Is this not a concern for search-sort-by?

Edit: I misunderstood this function and should've read more closely before commenting. You can disregard my underscore comment. It doesn't matter for query param presentation

Copy link
Member Author

Choose a reason for hiding this comment

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

This file was removed actually.

): Promise<SearchAPIResponse> {
try {
// Keep commented in case we need to simulate a delay to test loaders
await new Promise((resolve) => setTimeout(resolve, 6250));
Copy link
Member

Choose a reason for hiding this comment

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

This should be commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thanks.

className="maxh-15 margin-bottom-2 tablet:margin-bottom-0"
className="margin-bottom-2 tablet:margin-bottom-0"
Copy link
Member

Choose a reason for hiding this comment

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

Why was . maxh-15 removed? It's there so that the logo doesn't grow too large on mobile. Larger breakpoints add a max size. But on mobile the image will just fill the allotted space and be HUGE.

image

Copy link
Member Author

@acouch acouch Jul 23, 2024

Choose a reason for hiding this comment

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

The max width creates an error in the console:

warn-once.js:16 Image with src "/_next/static/media/grants-gov-logo.7f89963e.png" has either width or height modified, but not the other. If you use CSS to change the size of your image, also include the styles 'width: "auto"' or 'height: "auto"' to maintain the aspect ratio

I removed the update and I created a separate ticket for it since it is out of scope for this ticket: #165

@rylew1
Copy link
Collaborator

rylew1 commented Jul 8, 2024

Would be curious to see this diagram updated if you have time: https://drive.google.com/file/d/14iXrcg5EsEvnqqhUeVgR-5F_X8PASw4d/view?usp=sharing
image

const updateQueryParams = (
queryParamValue: string | Set<string>,
key: string,
queryTerm: string | null | undefined,
scroll = false,
Copy link
Member

Choose a reason for hiding this comment

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

scroll is a query param?

Copy link
Member Author

Choose a reason for hiding this comment

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

I documented the reason for this. It is implemented in the bottom pager so that when the user clicks the pager, they aren't left at the bottom of the screen when the results reload.

@acouch
Copy link
Member Author

acouch commented Jul 26, 2024

@rylew1
diagram of components

As noted above, the workflow is:

  1. request comes, static version of the page is rendered
  2. search page params are args to search page
  3. static client components are rendered on the server
  4. components within the suspense boundary ask for their data
  5. data is fetched from the server (no change here) and results are streamed to the client
  6. client sees results

When a user makes a change, nothing happens on the server. The query params are captured and pushed to update the new URL and the process from above is repeated.

@acouch acouch requested a review from andycochran July 29, 2024 14:28
@andycochran andycochran requested a review from btabaska July 29, 2024 14:29
@rylew1
Copy link
Collaborator

rylew1 commented Jul 29, 2024

Is there a way to get the draw.io link or a larger picture of the updated diagram?

@acouch
Copy link
Member Author

acouch commented Jul 30, 2024

import PageSEO from "src/components/PageSEO";
import { QueryParamData } from "src/services/search/searchfetcher/SearchFetcher";
import QueryProvider from "./QueryProvider";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be changed to import QueryProvider from "src/app/[locale]/search/QueryProvider"; to match expected import pattern

Copy link
Member Author

Choose a reason for hiding this comment

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

@btabaska if the file is in the same directory I'm not sure if it is better to use the full string or not. I created #91 to decide on a standard. Can we punt to that ticket?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, any of these nitpicks are just suggestions. Feel free to not incorporate into final feedback

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated anyway, we can figure out the convention in #91 and ideally enforce with a linter.

@btabaska
Copy link
Collaborator

General feedback- this PR is way too large and in the future we should consider creating team standards around PR size. OpenSource.net has a great article with research that shows that we should keep PRs between 10-250 lines of code to make reviewing more effective.

Copy link
Collaborator

@btabaska btabaska left a comment

Choose a reason for hiding this comment

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

Lots of small nitpicks across the changes. It was hard to review this as a whole since the changes are so sweeping. We should likely break up changes like this into 5-6 smaller more focused PRs in the future to make the review process more effective. Great changes overall though, and running it locally this seems like a major improvement. Bravo!

frontend/src/components/search/SearchBar.tsx Outdated Show resolved Hide resolved
import { Icon } from "@trussworks/react-uswds";
import { useSearchParamUpdater } from "../../hooks/useSearchParamUpdater";
import { useState } from "react";
import { sendGAEvent } from "@next/third-parties/google";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason that we are deprecating code changes for Google Analytics?

Copy link
Member Author

Choose a reason for hiding this comment

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

const { updateQueryParams } = useSearchParamUpdater();

const handleSubmit = () => {
updateQueryParams(inputValue, "query");
sendGAEvent("event", "search", { search_term: inputValue });
Copy link
Collaborator

Choose a reason for hiding this comment

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

See above, if we deprecate the GAEvent it will break Google Analytics support.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for noting. See comment above.

interface SearchFilterAccordionProps {
initialFilterOptions: FilterOption[];
options: FilterOption[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are your thoughts around "options" as the variable name? It might come across as confusing to developers working in the codebase in the future to have such a generic name. Since we have so many different types of options across the codebase, it might be more clear to keep the name "filterOptions" or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

@btabaska since the interface is SearchFilterAccordionProps it seems cleaner to me to have it as just options as it would follow that they are options for SearchFilterAccordion. I changed it because the "initial" didn't make sense anymore as these are not capturing state. It is great to keep in mind community devs and like filterOptions makes more sense for you so updating it.


import { FilterOption } from "../SearchFilterAccordion";
import { useState } from "react";
import { FilterOptionWithChildren } from "../SearchFilterAccordion";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be updated to use the absolute import path from src

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, thx.

{searchResultsLength} Opportunities
<h2
className="tablet-lg:grid-col-fill margin-top-5 tablet-lg:margin-top-2 tablet-lg:margin-bottom-0"
style={{ opacity: loading ? 0.5 : 1 }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this something that we can achieve by using our existing style or by creating a custom USWDS style? It is usually not best practice to style inline because it can cause styling creep over time that makes it hard to parse where CSS is being applied from.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, thanks for noting.

"use server";
import { getSearchFetcher } from "src/services/search/searchfetcher/SearchFetcherUtil";
import { QueryParamData } from "src/services/search/searchfetcher/SearchFetcher";
import SearchResultsHeader from "./SearchResultsHeader";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be updated to use an absolute path.

}

if (searchResults.data.length === 0) {
return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

These strings need to be migrated into locales so that translate will work on them. I don't think we want a pattern where some text is located there and some is located inline to the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@btabaska I agree, they were not translated originally so was trying to keep the scope of the PR down, even as large as it is. I created a follow up: #167

{maxPaginationError && (
<h4>
{
"You''re trying to access opportunity results that are beyond the last page of data."
Copy link
Collaborator

Choose a reason for hiding this comment

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

This string needs to be migrated into locales, we don't want inline text that will not be seen by translate and causes confusion on where we locate text.


sendGAEvent("event", "search", { key: finalQueryParamValue });
Copy link
Collaborator

Choose a reason for hiding this comment

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

"key" needs to be changed either to "search_term" to match what we have in GA or it needs to be updated in GA to "key". GA won't pick up or understand what "key" means on its own. I'd recommend we change it to search_term here since "key" is generic and means a lot of different things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, thanks!

@acouch
Copy link
Member Author

acouch commented Jul 30, 2024

@btabaska I agree but not sure how to break this up. The key items are to render the page statically, which wasn't possible w/o introducing the suspense items. Introducing the suspense items one at a time would take a ton of extra work. This takes advantage of the downtime.

@btabaska
Copy link
Collaborator

@btabaska I agree but not sure how to break this up. The key items are to render the page statically, which wasn't possible w/o introducing the suspense items. Introducing the suspense items one at a time would take a ton of extra work. This takes advantage of the downtime.

Totally fine, and agreed. I just wanted to point that out as a general concern. If we were in a live environment and not during downtime I might suggest a feature branch or something. However, in this instance it isn't a big deal.

@acouch acouch force-pushed the acouch/search-suspense branch from 94cbc76 to 57f9c81 Compare July 30, 2024 18:17
@acouch acouch force-pushed the acouch/search-suspense branch from e011b11 to 711c24c Compare August 6, 2024 16:12
@btabaska btabaska self-assigned this Aug 6, 2024
Copy link
Collaborator

@btabaska btabaska left a comment

Choose a reason for hiding this comment

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

LGTM!

@acouch acouch merged commit 9eb8aa2 into main Aug 6, 2024
12 checks passed
@acouch acouch deleted the acouch/search-suspense branch August 6, 2024 18:03
acouch added a commit that referenced this pull request Sep 18, 2024
Fixes #59

This makes the search page static and adds a suspense boundary for the
data being fetched by the server. The data comes from the API and is
called from 3 components:

* [`<SearchPaginationFetch
/>`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-9dbdda5096b97ad049cccea24c5a046581d26c151a6f94fcc32c05cb33ee9dee)
* [`<SearchResultsHeaderFetch
/>`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-14a084f66c050414cc2bbd0875256511630438971022073301bbfe91c4aa8cd1)
* [`<SearchResultsListFetch
/>`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-aabe6a7d19434a9b26199430bbcde5d31a0790aebc4cd844b922ac2fa1348dce)

This also simplifies the state model by pushing state changes directly
to the browser query params and rerendering the changed items. This
makes things a lot simpler and thus a lot of state management code is
removed and there results list is no longer wrapped in a form and
passing form refs between components. This is the recommended approach
by next:
https://nextjs.org/learn/dashboard-app/adding-search-and-pagination

There are several items that needed to be shared among the client
components: the query, total results count, and total pages. These are
wrapped in a `<QueryProvider />` that updates the state of these items.
This was added so that if someone enters a query in the text box and the
clicks a filter their query is not lost, so that the "N Opportunities"
text doesn't need to be rerendered when paging or sorting, and so that
the pager stays the same length when paging or sorting.

The data is fetched a couple of times in a duplicative fashion, however
this follows [NextJS best
practice](https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#sharing-data-between-components)
since the requests are cached.

The pager has been updated to reload only when there is a change in the
page length. Because of an issue with the way the pager renders, it is
unavailable while data is being updated:

<img width="1229" alt="image"
src="https://github.com/navapbc/simpler-grants-gov/assets/512243/a097b0e2-f646-43b5-bc5a-664db02780a2">

This is because the Truss React component [switches between a link and a
button as it
renders](https://github.com/trussworks/react-uswds/blob/main/src/components/Pagination/Pagination.tsx#L42)
and there isn't an option to supply query arguments, so if a user where
to click it they would lose the query params.

Overall this puts us on nice footing for the upcoming work using NextJS
best practice.
acouch added a commit that referenced this pull request Sep 18, 2024
Fixes #59

This makes the search page static and adds a suspense boundary for the
data being fetched by the server. The data comes from the API and is
called from 3 components:

* [`<SearchPaginationFetch
/>`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-9dbdda5096b97ad049cccea24c5a046581d26c151a6f94fcc32c05cb33ee9dee)
* [`<SearchResultsHeaderFetch
/>`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-14a084f66c050414cc2bbd0875256511630438971022073301bbfe91c4aa8cd1)
* [`<SearchResultsListFetch
/>`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-aabe6a7d19434a9b26199430bbcde5d31a0790aebc4cd844b922ac2fa1348dce)

This also simplifies the state model by pushing state changes directly
to the browser query params and rerendering the changed items. This
makes things a lot simpler and thus a lot of state management code is
removed and there results list is no longer wrapped in a form and
passing form refs between components. This is the recommended approach
by next:
https://nextjs.org/learn/dashboard-app/adding-search-and-pagination

There are several items that needed to be shared among the client
components: the query, total results count, and total pages. These are
wrapped in a `<QueryProvider />` that updates the state of these items.
This was added so that if someone enters a query in the text box and the
clicks a filter their query is not lost, so that the "N Opportunities"
text doesn't need to be rerendered when paging or sorting, and so that
the pager stays the same length when paging or sorting.

The data is fetched a couple of times in a duplicative fashion, however
this follows [NextJS best
practice](https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#sharing-data-between-components)
since the requests are cached.

The pager has been updated to reload only when there is a change in the
page length. Because of an issue with the way the pager renders, it is
unavailable while data is being updated:

<img width="1229" alt="image"
src="https://github.com/navapbc/simpler-grants-gov/assets/512243/a097b0e2-f646-43b5-bc5a-664db02780a2">

This is because the Truss React component [switches between a link and a
button as it
renders](https://github.com/trussworks/react-uswds/blob/main/src/components/Pagination/Pagination.tsx#L42)
and there isn't an option to supply query arguments, so if a user where
to click it they would lose the query params.

Overall this puts us on nice footing for the upcoming work using NextJS
best practice.
acouch added a commit to HHS/simpler-grants-gov that referenced this pull request Sep 18, 2024
Fixes #59

This makes the search page static and adds a suspense boundary for the
data being fetched by the server. The data comes from the API and is
called from 3 components:

* [`<SearchPaginationFetch
/>`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-9dbdda5096b97ad049cccea24c5a046581d26c151a6f94fcc32c05cb33ee9dee)
* [`<SearchResultsHeaderFetch
/>`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-14a084f66c050414cc2bbd0875256511630438971022073301bbfe91c4aa8cd1)
* [`<SearchResultsListFetch
/>`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-aabe6a7d19434a9b26199430bbcde5d31a0790aebc4cd844b922ac2fa1348dce)

This also simplifies the state model by pushing state changes directly
to the browser query params and rerendering the changed items. This
makes things a lot simpler and thus a lot of state management code is
removed and there results list is no longer wrapped in a form and
passing form refs between components. This is the recommended approach
by next:
https://nextjs.org/learn/dashboard-app/adding-search-and-pagination

There are several items that needed to be shared among the client
components: the query, total results count, and total pages. These are
wrapped in a `<QueryProvider />` that updates the state of these items.
This was added so that if someone enters a query in the text box and the
clicks a filter their query is not lost, so that the "N Opportunities"
text doesn't need to be rerendered when paging or sorting, and so that
the pager stays the same length when paging or sorting.

The data is fetched a couple of times in a duplicative fashion, however
this follows [NextJS best
practice](https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#sharing-data-between-components)
since the requests are cached.

The pager has been updated to reload only when there is a change in the
page length. Because of an issue with the way the pager renders, it is
unavailable while data is being updated:

<img width="1229" alt="image"
src="https://github.com/navapbc/simpler-grants-gov/assets/512243/a097b0e2-f646-43b5-bc5a-664db02780a2">

This is because the Truss React component [switches between a link and a
button as it
renders](https://github.com/trussworks/react-uswds/blob/main/src/components/Pagination/Pagination.tsx#L42)
and there isn't an option to supply query arguments, so if a user where
to click it they would lose the query params.

Overall this puts us on nice footing for the upcoming work using NextJS
best practice.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Update Search Page so Page Loads Statically and Results Are Pulled in Async
4 participants