-
Notifications
You must be signed in to change notification settings - Fork 0
[Issue #96]: Opportunity listing page (first pass) #97
Changes from 5 commits
805b253
3ed6893
9bc51db
f95d3bc
de00b48
ea45074
9a11a74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
import { Metadata } from "next"; | ||
import NotFound from "../../../not-found"; | ||
import OpportunityListingAPI from "../../../api/OpportunityListingAPI"; | ||
import { getTranslations } from "next-intl/server"; | ||
|
||
export async function generateMetadata() { | ||
const t = await getTranslations({ locale: "en" }); | ||
const meta: Metadata = { | ||
title: t("OpportunityListing.page_title"), | ||
description: t("OpportunityListing.meta_description"), | ||
}; | ||
return meta; | ||
} | ||
|
||
export default async function OpportunityListing({ | ||
params, | ||
}: { | ||
params: { id: string }; | ||
}) { | ||
const id = Number(params.id); | ||
|
||
// Opportunity id needs to be a number greater than 1 | ||
if (isNaN(id) || id < 0) { | ||
return <NotFound />; | ||
} | ||
|
||
const api = new OpportunityListingAPI(); | ||
let opportunity; | ||
try { | ||
opportunity = await api.getOpportunityById(id); | ||
} catch (error) { | ||
console.error("Failed to fetch opportunity:", error); | ||
return <NotFound />; | ||
} | ||
|
||
if (!opportunity.data) { | ||
return <NotFound />; | ||
} | ||
|
||
return ( | ||
<div className="grid-container"> | ||
<div className="grid-row margin-y-4"> | ||
<div className="usa-table-container"> | ||
<table className="usa-table usa-table--borderless margin-x-auto width-full maxw-desktop-lg"> | ||
<thead> | ||
<tr> | ||
<th>Field Name</th> | ||
<th>Data</th> | ||
</tr> | ||
</thead> | ||
<tbody> | ||
{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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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)
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @chouinar I added this: |
||
</div> | ||
</div> | ||
</div> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,3 @@ | ||
// This server-only package is recommended by Next.js to ensure code is only run on the server. | ||
// It provides a build-time error if client-side code attempts to invoke the code here. | ||
// Since we're pulling in an API Auth Token here, this should be server only | ||
// https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#keeping-server-only-code-out-of-the-client-environment | ||
import "server-only"; | ||
|
||
import { | ||
|
@@ -69,7 +65,7 @@ export default abstract class BaseApi { | |
basePath: string, | ||
namespace: string, | ||
subPath: string, | ||
queryParamData: QueryParamData, | ||
queryParamData?: QueryParamData, | ||
body?: JSONRequestBody, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all queryParamData / searchInput references need to be an optional param because they're only used for search ( |
||
options: { | ||
additionalHeaders?: HeadersDict; | ||
|
@@ -109,7 +105,7 @@ export default abstract class BaseApi { | |
private async sendRequest( | ||
url: string, | ||
fetchOptions: RequestInit, | ||
queryParamData: QueryParamData, | ||
queryParamData?: QueryParamData, | ||
) { | ||
let response: Response; | ||
let responseBody: SearchAPIResponse; | ||
|
@@ -189,19 +185,21 @@ function createRequestBody(payload?: JSONRequestBody): XMLHttpRequestBodyInit { | |
*/ | ||
export function fetchErrorToNetworkError( | ||
error: unknown, | ||
searchInputs: QueryParamData, | ||
searchInputs?: QueryParamData, | ||
) { | ||
// Request failed to send or something failed while parsing the response | ||
// Log the JS error to support troubleshooting | ||
console.error(error); | ||
return new NetworkError(error, searchInputs); | ||
return searchInputs | ||
? new NetworkError(error, searchInputs) | ||
: new NetworkError(error); | ||
} | ||
|
||
function handleNotOkResponse( | ||
response: SearchAPIResponse, | ||
message: string, | ||
status_code: number, | ||
searchInputs: QueryParamData, | ||
searchInputs?: QueryParamData, | ||
) { | ||
const { errors } = response; | ||
if (isEmpty(errors)) { | ||
|
@@ -218,7 +216,7 @@ function handleNotOkResponse( | |
const throwError = ( | ||
message: string, | ||
status_code: number, | ||
searchInputs: QueryParamData, | ||
searchInputs?: QueryParamData, | ||
firstError?: APIResponseError, | ||
) => { | ||
console.log("Throwing error: ", message, status_code, searchInputs); | ||
|
@@ -246,9 +244,9 @@ const throwError = ( | |
default: | ||
throw new ApiRequestError( | ||
error, | ||
searchInputs, | ||
"APIRequestError", | ||
status_code, | ||
searchInputs, | ||
); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import "server-only"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. only ever run this code on the next server |
||
|
||
import BaseApi from "./BaseApi"; | ||
|
||
export default class OpportunityListingAPI extends BaseApi { | ||
get basePath(): string { | ||
return process.env.API_URL || ""; | ||
} | ||
|
||
get namespace(): string { | ||
return "opportunities"; | ||
} | ||
|
||
async getOpportunityById(opportunityId: number) { | ||
const subPath = `${opportunityId}`; | ||
const response = await this.request( | ||
"GET", | ||
this.basePath, | ||
this.namespace, | ||
subPath, | ||
); | ||
return response; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. allow pa11y to run on the opportunity listing page by just setting any |
||
: "/"; | ||
return route.replace(/\/\//g, "/"); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
describe("getNextRoutes", () => { | ||
beforeEach(() => { | ||
jest.resetAllMocks(); | ||
|
@@ -30,6 +31,7 @@ describe("getNextRoutes", () => { | |
"/newsletter/confirmation", | ||
"/newsletter", | ||
"/newsletter/unsubscribe", | ||
"/opportunity/1", | ||
"/", | ||
"/process", | ||
"/research", | ||
|
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'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.
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.
We can punt this consideration for #85 .
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.
🧵 here https://nava.slack.com/archives/C057K146W8H/p1718912909890579