[Issue #8596] Create Opportunity page layout and components#8991
[Issue #8596] Create Opportunity page layout and components#8991
Conversation
| <> | ||
| <GridContainer> | ||
| <PageHeader/> | ||
| <CreateOpportunityPage1 |
There was a problem hiding this comment.
Should avoid arbitrary variable naming - what about CreateOpportunityForm?
| ]; | ||
|
|
||
| // Field level error messages | ||
| let oppNbrVldtnError = ""; |
There was a problem hiding this comment.
we should track things like this, if necessary, in component state rather than in module globals. React will handle this better than the module will (as the file may be re-loaded at weird times, and state may be reset as result)
|
|
||
| // All labels/text on this page with support for international languages | ||
| const t = useTranslations("CreateOpportunity.CreateOpportunityPage1"); | ||
| const oppNbrLabel = t("opportunityNumber"); |
There was a problem hiding this comment.
Since defining variables like this leads to some bloat in the file, I'd rather follow the usual pattern and call t directly where these values are being used
| const charsAllowed255 = t("charactersAllowed255"); | ||
|
|
||
| // Agencies: sort alphabetically, convert to key-value pairs, set the default | ||
| const sortedAgencies = [...userAgencies].sort((a, b) => |
There was a problem hiding this comment.
my guess is that the agency endpoint supports sorting, will be easier to deal with things coming in pre-sorted
| key: agency.agency_id, | ||
| value: agency.agency_name, | ||
| })); | ||
| keyValueList.forEach((item) => { |
There was a problem hiding this comment.
as it is this will run on each render - can you wrap this and the keyValueList mapping in a useEffect?
| }; | ||
|
|
||
| // Custom CSS for the cancel button | ||
| const cancelButtonStyle: React.CSSProperties = { |
There was a problem hiding this comment.
unless there's a really compelling reason we should not use inline styling
| const handleCancel = () => { | ||
| redirect("/opportunities") | ||
| }; | ||
| const handleSave = () => { |
There was a problem hiding this comment.
let's use a server action for this - that may change the general approach here a little
| export const defaultFeatureFlags: FeatureFlags = { | ||
| applyFormPrototypeOff: false, | ||
| opportunitiesListOff: false, | ||
| createOpportunityOff: false, |
There was a problem hiding this comment.
if this PR is creating a new feature flag, it should also update the terraform, see https://github.com/HHS/simpler-grants-gov/blob/main/documentation/frontend/featureFlags.md
| @@ -0,0 +1,236 @@ | |||
| // These fields are meant to be reusable and provide a consistent look and feel for all pages. | |||
There was a problem hiding this comment.
I like the idea here a lot, but I think we should talk about this as a group before jumping in. It probably makes the most sense to use custom / barebones implementation rather than shared / general components at this point,
2 participants
Summary
Work for #8596
Changes proposed
Context for reviewers
Validation steps
Acceptance Criteria: