-
Notifications
You must be signed in to change notification settings - Fork 0
Add suspense boundary around search page results #101
Conversation
Coverage report for
|
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
// Pass 'true' to handle the special case of splitting 'funding-instrument-'after the second dash | ||
return this.getValuesByKeyPrefix("funding-instrument-", true); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks.
frontend/src/components/Footer.tsx
Outdated
className="maxh-15 margin-bottom-2 tablet:margin-bottom-0" | ||
className="margin-bottom-2 tablet:margin-bottom-0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
Would be curious to see this diagram updated if you have time: https://drive.google.com/file/d/14iXrcg5EsEvnqqhUeVgR-5F_X8PASw4d/view?usp=sharing |
const updateQueryParams = ( | ||
queryParamValue: string | Set<string>, | ||
key: string, | ||
queryTerm: string | null | undefined, | ||
scroll = false, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
3f97eda
to
df65dbc
Compare
As noted above, the workflow is:
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. |
Is there a way to get the draw.io link or a larger picture of the updated diagram? |
import PageSEO from "src/components/PageSEO"; | ||
import { QueryParamData } from "src/services/search/searchfetcher/SearchFetcher"; | ||
import QueryProvider from "./QueryProvider"; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this 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!
import { Icon } from "@trussworks/react-uswds"; | ||
import { useSearchParamUpdater } from "../../hooks/useSearchParamUpdater"; | ||
import { useState } from "react"; | ||
import { sendGAEvent } from "@next/third-parties/google"; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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[]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 }} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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 ( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{maxPaginationError && ( | ||
<h4> | ||
{ | ||
"You''re trying to access opportunity results that are beyond the last page of data." |
There was a problem hiding this comment.
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 }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, thanks!
@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. |
94cbc76
to
57f9c81
Compare
e011b11
to
711c24c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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.
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.
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.
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:
<SearchPaginationFetch />
<SearchResultsHeaderFetch />
<SearchResultsListFetch />
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:
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.