[Issue #96]: Opportunity listing page (first pass) #97

rylew1 · 2024-06-18T23:03:13Z

Summary

Fixes #96

Time to review: 10 min

Changes proposed

Add new id-based opportunity page
Add new OpportunityListingAPI class extended from BaseAPI
Make searchInputs/QueryParamData in BaseAPI and errors.ts optional params (only used for search page)
Update sitemap to replace [id] in url with 1
Add test coverage

Context for reviewers

Page just shows a table with simple results:

Screen.Recording.2024-06-18.at.7.00.46.PM.mov

github-actions · 2024-06-19T01:46:49Z

Coverage report for `./frontend`

St.^❔	Category	Percentage	Covered / Total
🟢	Statements	83.87% (+0.3% 🔼)	879/1048
🟡	Branches	68.25% (-0.14% 🔻)	230/337
🟡	Functions	76.11% (+0.09% 🔼)	172/226
🟢	Lines	83.45% (+0.32% 🔼)	827/991

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / OpportunityListingAPI.ts	88.89%	100%	75%	88.89%
🟢	`...` / isSummary.ts	100%	100%	100%	100%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🔴	src/errors.ts	51.72%	50% (-7.14% 🔻)	50%	51.72%

Test suite run success

175 tests passing in 57 suites.

Report generated by 🧪jest coverage report action from 9a11a74

rylew1 · 2024-06-19T01:55:47Z

frontend/src/app/api/BaseApi.ts

@@ -69,7 +65,7 @@ export default abstract class BaseApi {
    basePath: string,
    namespace: string,
    subPath: string,
-    queryParamData: QueryParamData,
+    queryParamData?: QueryParamData,


all queryParamData / searchInput references need to be an optional param because they're only used for search (SearchOpportunityAPI class) and not when the derived OpportunityListingAPI class is used.

rylew1 · 2024-06-19T01:56:53Z

frontend/src/app/api/OpportunityListingAPI.ts

@@ -0,0 +1,24 @@
+import "server-only";


only ever run this code on the next server

rylew1 · 2024-06-19T01:58:35Z

frontend/src/utils/getRoutes.ts

@@ -32,6 +32,7 @@ export function getNextRoutes(src: string): string[] {
          .replace("/page.tsx", "")
          .replace(/\[locale\]/g, "")
          .replace(/\\/g, "/")
+          .replace(/\[id\]/g, "1") // for id-based routes like /opportunity/[id]


allow pa11y to run on the opportunity listing page by just setting any [id] found to 1 - resulting in /opportunity/1

rylew1 · 2024-06-19T01:59:31Z

frontend/tests/utils/getRoutes.test.ts

@@ -12,6 +12,7 @@ jest.mock("../../src/utils/getRoutes", () => {

 const mockedListPaths = listPaths as jest.MockedFunction<typeof listPaths>;

+// TODO: https://github.com/navapbc/simpler-grants-gov/issues/98


this test is a little wonky rn - it's not actually mocking properly so it pulls what the sitemap actually pulls (so we have to update the test every time there's a new route - which is not really testing the functions in getRoutes in isolation) - so there's a ticket to come back to this

btabaska

LGTM

chouinar · 2024-06-20T15:49:30Z

frontend/src/app/[locale]/opportunity/[id]/page.tsx

+              {Object.entries(opportunity.data).map(([key, value]) => (
+                <tr key={key}>
+                  <td className="word-wrap">{key}</td>
+                  <td className="word-wrap">{JSON.stringify(value)}</td>
+                </tr>
+              ))}
+            </tbody>
+          </table>


Could you add a small bit of special logic for the summary field so that it doesn't print out as the full JSON, but instead pulls out each of the fields? Just to make it look a little bit nicer / make it look like we've done a lot more and are truly just waiting on shoving it into whatever designs.

So, rather than summary with a value of { ... } (the whole object), do something like (in hacky python pseudo-code):

for key, value in obj.items(): if key == "summary": for summary_key, summary_value in value.items(): print(summary_key) print(JSON.stringify(summary_value) else: print(key) print(value)

Thanks @chouinar I added this:

acouch · 2024-06-20T19:09:45Z

frontend/src/app/[locale]/opportunity/[id]/page.tsx

+  return meta;
+}
+
+export default async function OpportunityListing({


I'm still catching up on whether or not making a page async affects ability to statically render the page. The opportunity pages are the same for everyone and rarely update so should not be rebuilt for every single request. Statically rendering seems like the easiest way to achieve that.

We can punt this consideration for #85 .

🧵 here https://nava.slack.com/archives/C057K146W8H/p1718912909890579

Fixes HHS#2068 - Add new id-based opportunity page - Add new `OpportunityListingAPI` class extended from `BaseAPI` - Make `searchInputs`/`QueryParamData` in `BaseAPI` and `errors.ts` optional params (only used for search page) - Update sitemap to replace [id] in url with 1 - Add test coverage

Fixes #2068 - Add new id-based opportunity page - Add new `OpportunityListingAPI` class extended from `BaseAPI` - Make `searchInputs`/`QueryParamData` in `BaseAPI` and `errors.ts` optional params (only used for search page) - Update sitemap to replace [id] in url with 1 - Add test coverage

initial commit

805b253

rylew1 requested review from andycochran and acouch as code owners June 18, 2024 23:03

rylew1 marked this pull request as draft June 18, 2024 23:03

github-actions bot added typescript frontend labels Jun 18, 2024

rylew1 added 2 commits June 18, 2024 18:02

fix error param order

3ed6893

patch test

9bc51db

remove console log

f95d3bc

rylew1 commented Jun 19, 2024

View reviewed changes

revert bodyParams to body

de00b48

rylew1 commented Jun 19, 2024

View reviewed changes

rylew1 marked this pull request as ready for review June 19, 2024 01:59

rylew1 requested review from btabaska and chouinar June 19, 2024 02:00

btabaska reviewed Jun 20, 2024

View reviewed changes

btabaska self-requested a review June 20, 2024 13:55

rylew1 changed the title ~~[Issue #96]: Opportunity listing page~~ [Issue #96]: Opportunity listing page (first pass) Jun 20, 2024

chouinar reviewed Jun 20, 2024

View reviewed changes

acouch reviewed Jun 20, 2024

View reviewed changes

rylew1 added 2 commits June 20, 2024 12:35

add typing and fix ts errors

ea45074

add test coverage

9a11a74

acouch approved these changes Jun 20, 2024

View reviewed changes

rylew1 merged commit 4c877fa into main Jun 20, 2024
12 checks passed

rylew1 deleted the rylew/96-opportunity-listing-page branch June 20, 2024 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #96]: Opportunity listing page (first pass) #97

[Issue #96]: Opportunity listing page (first pass) #97

rylew1 commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 19, 2024 •

edited

Loading

rylew1 Jun 19, 2024

rylew1 Jun 19, 2024 •

edited

Loading

rylew1 Jun 19, 2024

rylew1 Jun 19, 2024

btabaska left a comment •

edited

Loading

chouinar Jun 20, 2024 •

edited

Loading

rylew1 Jun 20, 2024

acouch Jun 20, 2024

acouch Jun 20, 2024

rylew1 Jun 20, 2024

		@@ -12,6 +12,7 @@ jest.mock("../../src/utils/getRoutes", () => {

		const mockedListPaths = listPaths as jest.MockedFunction<typeof listPaths>;

		// TODO: https://github.com/navapbc/simpler-grants-gov/issues/98

[Issue #96]: Opportunity listing page (first pass) #97

[Issue #96]: Opportunity listing page (first pass) #97

Conversation

rylew1 commented Jun 18, 2024 • edited Loading

Summary

Time to review: 10 min

Changes proposed

Context for reviewers

github-actions bot commented Jun 19, 2024 • edited Loading

Coverage report for ./frontend

Test suite run success

rylew1 Jun 19, 2024

Choose a reason for hiding this comment

rylew1 Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

rylew1 Jun 19, 2024

Choose a reason for hiding this comment

rylew1 Jun 19, 2024

Choose a reason for hiding this comment

btabaska left a comment • edited Loading

Choose a reason for hiding this comment

chouinar Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

rylew1 Jun 20, 2024

Choose a reason for hiding this comment

acouch Jun 20, 2024

Choose a reason for hiding this comment

acouch Jun 20, 2024

Choose a reason for hiding this comment

rylew1 Jun 20, 2024

Choose a reason for hiding this comment

rylew1 commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 19, 2024 •

edited

Loading

Coverage report for `./frontend`

rylew1 Jun 19, 2024 •

edited

Loading

btabaska left a comment •

edited

Loading

chouinar Jun 20, 2024 •

edited

Loading