[ Issue 361 ] Funding Opportunity Content #377

SammySteiner · 2023-08-17T16:10:40Z

Summary

Fixes #361

Time to review: 15 min

Changes proposed

Added funding opportunity content to index page
Created pdf container component
created stories for funding opportunity content component and pdf container component
tests and test coverage for funding opportunity content component and pdf container component
Added custom css class with slight hover animation for pdfs

Context for reviewers

Ignored Feedback form per discussion with @andycochran
Discussed image breakpoints with @andycochran for pdfs to confirm 4 -> 2 -> 1 for different screen sizes
Discussed where images should be served with Andy and decided root/docs/pdf_file was the best user experience, though this may create some duplication with where the pdfs are currently stored
If/when pdfs are updated, if the front page or two are changed, we may need to update the images as well.
Had to add screen breakpoints to our theme to add tabletLg for the additional image breakpoint

Additional information

Screenshots, GIF demos, code examples or output to help show the changes working as expected.

Local test and coverage

index page desktop

index page mobile

storybook

andycochran · 2023-08-17T18:43:33Z

frontend/src/components/PdfContainer.tsx

+
+const PdfContainer = ({ file, image, alt }: pdf) => {
+  return (
+    <Grid className="margin-bottom-2" tablet={{ col: 6 }} tabletLg={{ col: 3 }}>


nit: PdfContainer seems like the wrong name. It doesn't contain any PDFs. It links to PDFs, but technically that doesn't even have to be the case; you could pass it whatever link (file doesn't even have to be a file). Functionally this component is more of an "image link." You might even move this grid element into the nofoPdfs.map in the funding content and let this component just worry about making image links.

Makes sense to me. I renamed it NofoImageLInk.

frontend/src/styles/_uswds-theme-custom-styles.scss

Co-authored-by: Andy Cochran <[email protected]>

andycochran · 2023-08-17T18:50:29Z

frontend/src/styles/_uswds-theme.scss

+    "widescreen": false,
+    // 1400px
+  ),
+


Is the formatter moving these inline comments to their own line? I find this much easier to read:

Yeah, that was the formatter.

Might be a setting that allows inline CSS comments. 🤔

What is this doing? Are we overriding the defaults?

By default only mobile-lg, tablet, and desktop breakpoints are enabled. This PR overrides the breakpoint map to also enable tablet-lg.

…so that we can have all 4 PDF links beside each other on screens slightly smaller than desktop. Otherwise the link images are HUGE.

Seems like prettier is super opinionated on not having comments on the same line as code, so much so that there is no option to disable it for a file, and for some reason, // prettier-ignore ir /* prettier-ignore */ isn't working on this block of scss code: prettier/prettier#807

Maybe we don't need the inline comments.

/* Override default breakpoints, see https://designsystem.digital.gov/utilities/layout-grid/#utilities-settings */ $theme-utility-breakpoints: ( "card": false, "card-lg": false, "mobile": false, "mobile-lg": true, "tablet": true, "tablet-lg": true, "desktop": true, "desktop-lg": false, "widescreen": false, ),

kk, let's go with that.

andycochran · 2023-08-17T19:28:53Z

frontend/src/components/FundingContent.tsx

+  const { t } = useTranslation("common", { keyPrefix: "Index" });
+
+  return (
+    <div className="bg-base-lightest">


Can we add some dynamic top/bottom padding to this section? LIke what @daphnegold did on the hero section:

className="bg-primary desktop:padding-y-15 tablet:padding-y-2 padding-y-1"

Not as much as "15" but this same technique.

sounds good, I added className="desktop:padding-y-4 tablet:padding-y-2 padding-y-1" to both the funding and goal content sections.

daphnegold · 2023-08-17T22:31:28Z

frontend/package-lock.json

There are a lot of additional packages in here, is that intentional or maybe a remnant of react-pdf?

Yeah, that must have been a hold over from the react-pdf experimentation. Thanks for catching.

daphnegold · 2023-08-17T22:41:53Z

frontend/src/components/FundingContent.tsx

+          <h3>{t("fo_title_2")}</h3>
+        </Grid>
+        <Grid>
+          <p className="usa-intro">{t("fo_paragraph_3")}</p>


I noticed this in the Goal Content component too, that the fonts are kinda wonky and we're swapping between serif and sans. Just an observation we may want to address now or in the future.

The cause according to testing locally is the usa-intro class. I'm guessing in some cases it applies a different font palette that overrides our current one? Not entirely sure, should we remove?

Once we have the whole page together, I'll take a pass at tidying up any typography and whitespace issues. Within the context of a responsive layout, I'll be able to better specify optimal size/padding/etc. (That's harder and more tedious to do in Figma unless you make like 5 versions of the page at various widths.)

(In Figma, I failed to represent the .usa-intro content with the right serif font. Not a problem that this doesn't match what's in Figma.)

daphnegold

Only thing I'd request is to make sure the package-lock.json is reflecting reality because there's a lot of new stuff in there, possibly erroneously

daphnegold · 2023-08-17T22:44:30Z

frontend/src/styles/_uswds-theme.scss

+    "widescreen": false,
+    // 1400px
+  ),
+


What is this doing? Are we overriding the defaults?

daphnegold · 2023-08-17T22:46:10Z

frontend/tests/pages/index.test.tsx

@@ -37,7 +37,7 @@ describe("Index", () => {

  it("passes accessibility scan", async () => {
    const { container } = render(<Index />);
-    const results = await axe(container);
+    const results = await waitFor(() => axe(container));


Just curious why this was added for my own knowledge :D

Because we're using the next/link Link component, that returns a promise, I think, so without the waitFor, the axe was not able to see the entire page. As a result it logged a bunch of warnings.

daphnegold · 2023-08-17T22:48:26Z

frontend/src/components/FundingContent.tsx

+          <p className="usa-intro">{t("fo_paragraph_3")}</p>
+        </Grid>
+        <Grid row gap>
+          {nofoPdfs.map((pdf) => (


I love the way you didn't hardcode these in here and we can swap them out! 🥇

daphnegold and others added 30 commits July 26, 2023 15:25

Create story file for header

9837319

Start modifying Header.tsx to fit our designs

cfd7805

Create optional props to align header with figma designs

f92da57

Fix linter

f2e3753

Fill out stories

b71e4e5

Ooo fun, run format

b262a5e

Fix showMenu logic, only if navItems

1cdedb6

Testing

cf4388b

Add design link to Story

a3a442c

Design addon for storybook, now see figma designs right in the story!

b6f392f

Merge branch 'main' into daphnegold/issue-290-header

812afb3

alert component, story, test

9834088

Merge branch 'main' into sammysteiner/issue-360-alert

50ad308

format

341428b

typp

ca9f485

Dry parameters

65e9665

content, styles, tests added for index

3b7b713

npm run format

c1ae0ba

Merge branch 'main' into sammysteiner/issue-358-goal-content-ss

4f0a1f4

lint and merge conflicts

42223b2

pull goal content into a component

418ff30

gc test

0eda427

gc story

9c0f623

Merge branch 'main' into sammysteiner/issue-358-goal-content-ss

1488963

remove grid container padding

89ce43f

tablet breakpoint for columns

a085bc1

remove custom styles

23159a1

remove vertical bar from alert

e66989d

updated alert component

206f524

andy inspired more elegant styling

dd068f3

SammySteiner added 3 commits August 17, 2023 11:58

responsive div only needed for next image fill

27a97e8

removed unnecessary dynamic import

54e0794

Merge branch 'main' into sammysteiner/issue-361-fo-content

2e83d9d

SammySteiner marked this pull request as ready for review August 17, 2023 16:26

SammySteiner requested review from daphnegold, andycochran and acouch August 17, 2023 16:27

SammySteiner added the topic: frontend label Aug 17, 2023

sumiat added this to the Static site launch with NOFO content milestone Aug 17, 2023

andycochran reviewed Aug 17, 2023

View reviewed changes

frontend/src/styles/_uswds-theme-custom-styles.scss Outdated Show resolved Hide resolved

Update frontend/src/styles/_uswds-theme-custom-styles.scss

64d5138

Co-authored-by: Andy Cochran <[email protected]>

andycochran reviewed Aug 17, 2023

View reviewed changes

Merge branch 'main' into sammysteiner/issue-361-fo-content

fb7ec1c

andycochran reviewed Aug 17, 2023

View reviewed changes

daphnegold reviewed Aug 17, 2023

View reviewed changes

SammySteiner added 3 commits August 18, 2023 10:13

unmodified package-lock.json

8fcead7

content section padding

dc17198

renamed pdf container

1f57f16

SammySteiner requested review from andycochran and daphnegold August 18, 2023 14:27

andycochran approved these changes Aug 18, 2023

View reviewed changes

removed screen size comments

8f9e7bc

SammySteiner merged commit e29f05e into main Aug 18, 2023

SammySteiner deleted the sammysteiner/issue-361-fo-content branch August 18, 2023 15:04

sumiat modified the milestones: Static site launch with NOFO content, Foundational UI Aug 24, 2023

sumiat mentioned this pull request Aug 29, 2023

[10k]: Foundational UI #60

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ Issue 361 ] Funding Opportunity Content #377

[ Issue 361 ] Funding Opportunity Content #377

SammySteiner commented Aug 17, 2023 •

edited

Loading

andycochran Aug 17, 2023 •

edited

Loading

SammySteiner Aug 18, 2023

andycochran Aug 17, 2023

SammySteiner Aug 17, 2023

andycochran Aug 17, 2023

daphnegold Aug 17, 2023

andycochran Aug 18, 2023

andycochran Aug 18, 2023

SammySteiner Aug 18, 2023

andycochran Aug 18, 2023

SammySteiner Aug 18, 2023

andycochran Aug 17, 2023

SammySteiner Aug 18, 2023

daphnegold Aug 17, 2023

SammySteiner Aug 18, 2023

daphnegold Aug 17, 2023 •

edited

Loading

andycochran Aug 18, 2023

andycochran Aug 18, 2023

daphnegold left a comment

daphnegold Aug 17, 2023

daphnegold Aug 17, 2023

SammySteiner Aug 18, 2023

daphnegold Aug 17, 2023

[ Issue 361 ] Funding Opportunity Content #377

[ Issue 361 ] Funding Opportunity Content #377

Conversation

SammySteiner commented Aug 17, 2023 • edited Loading

Summary

Time to review: 15 min

Changes proposed

Context for reviewers

Additional information

Local test and coverage

index page desktop

index page mobile

storybook

andycochran Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daphnegold Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daphnegold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SammySteiner commented Aug 17, 2023 •

edited

Loading

andycochran Aug 17, 2023 •

edited

Loading

daphnegold Aug 17, 2023 •

edited

Loading