-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add footer, slim footer story example #107
Conversation
secondary: React.ReactNode | ||
} | ||
|
||
// TODO: Add in "Return to Top" handling |
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.
How should this be implemented?
returnToTop
is an optional prop for the link- This component just renders
children
I didn't render children initially because it seemed the Footer
should also encapsulate styles, specifically ofprimary
and secondary
.
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 agree that primary
and secondary
is the way to go, since all of the USWDS footers divide up the content that way pretty explicitly. What if we had an optional onReturnToTop
fn prop that, if it exists renders the Return to top link and hooks up the event handler?
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.
Made returnToTop
a component when I realized that if I only made the handler a prop I am hardcoding English, and that specific phrase, into our repo. I'll make an issue about lang and i18n so we can discuss.
return ( | ||
<div className={containerClasses}> | ||
<div className="grid-col-auto"> | ||
<img className="usa-footer__logo-img" src="#" alt="img alt text" /> |
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 we add a mock logo image file to our library? It might be nicer for use in stories.
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.
Yeah! we could use the empty circle used on USWDS for something easy? https://designsystem.digital.gov/components/footer/
as long as it's only imported to the .stories.tsx
file it won't be included in the packaged library.
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.
Just pushed a couple more commits to round out this PR. Ended up adjusting the npm run storybook
script to get the static logo image loading properly. Followed this approach.
} | ||
) | ||
|
||
const items = Array(4).fill({ |
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.
🤯 TIL about Array.fill
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 don't know if maybe you were planning to do this later but we could probably create sub components scoped to the Footer, like Address, Logo, ContactInfo
?
looking good!
return ( | ||
<div className={containerClasses}> | ||
<div className="grid-col-auto"> | ||
<img className="usa-footer__logo-img" src="#" alt="img alt text" /> |
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.
Yeah! we could use the empty circle used on USWDS for something easy? https://designsystem.digital.gov/components/footer/
as long as it's only imported to the .stories.tsx
file it won't be included in the packaged library.
secondary: React.ReactNode | ||
} | ||
|
||
// TODO: Add in "Return to Top" handling |
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 agree that primary
and secondary
is the way to go, since all of the USWDS footers divide up the content that way pretty explicitly. What if we had an optional onReturnToTop
fn prop that, if it exists renders the Return to top link and hooks up the event handler?
@suzubara Yes about refactoring out components. Added FYI planning to add |
- serve static files via directory as per storybook docs - include story for slim logo only, other types to be added in future
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.
looking real good, I'm approving since this isn't going into develop yet and most of my comments are just ponderings & minor suggestions
src/components/Footer/Footer.tsx
Outdated
) | ||
|
||
return ( | ||
<footer className={classes}> |
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.
do we also want to include {...footerAttributes}
here to pass along anything else?
<div className="usa-footer__primary-section">{primary}</div> | ||
|
||
<div className="usa-footer__secondary-section"> | ||
<div className="grid-container">{secondary}</div> |
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.
why not leave <div className="grid-container">
to be part of the secondary
prop as well?
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.
Because a grid-container
is not always used to wrap the secondary content (in the other size types).
src/components/Footer/Logo/Logo.tsx
Outdated
<div className="grid-col-auto">{heading}</div> | ||
</> | ||
) : ( | ||
<>{image}</> |
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 think since the container is a grid-row
regardless of whether or not there's a heading, the image
should still be inside of a <div className="grid-col-auto">
?
) | ||
} | ||
|
||
const returnToTop = ( |
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.
Do you think this should be made a subcomponent as well? it could take an onClick handler (and i18n text)
* Add footer, slim footer story example * Add Logo, static file loading * Add Address
* Add footer, slim footer story example * Add Logo, static file loading * Add Address
* Add footer, slim footer story example * Add Logo, static file loading * Add Address
* Add footer, slim footer story example * Add Logo, static file loading * Add Address
* Add footer, slim footer story example * Add Logo, static file loading * Add Address
* Add footer, slim footer story example * Add Logo, static file loading * Add Address
* Add footer, slim footer story example * Add Logo, static file loading * Add Address
<Footer />
The footer displays primary content (upper section) and secondary content (lower section). This PR adds the initial component and a story example for displaying the slim footer.
PR (1 of 3) for #78