Skip to content
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

feat: Add compositional Banner components #2184

Merged
merged 16 commits into from
Sep 23, 2022
Merged

feat: Add compositional Banner components #2184

merged 16 commits into from
Sep 23, 2022

Conversation

brandonlenz
Copy link
Contributor

@brandonlenz brandonlenz commented Aug 8, 2022

Summary

Allow consumers to build custom Banners (say if a consumer needed a banner for a state government website)

This change is inspired by the DOL project which is not open source so I cannot link it here, but in short, we needed a custom Banner which GovBanner didn't satisfy the needs of.

I made the choice to leave the existing GovBanner in place because it's a nice convenience for consumers that want the default banner, and there's no easy way to introduce the breaking change workaround of removing it and then telling consumers to build a lengthy compositional component (see the example) just to replace what they had. This way, there's no breaking changes, consumers are happy, and they have more options to boot.

Related Issues or PRs

closes: #2183
closes: #1836

How To Test

  1. Verify GovBanner has not changed
    • There is a snapshot jest test that does this for us. Note the only change is the use of the inline svg, which actually matches the USWDS banner component code more closely than GovBanner previously did.
    • Note that this means that the exported uswds lock svg isn't used. That is intentional, so that props can be passed in to the inline svg (title, description, so that consumers can pass in localized strings)
    • The lack of a Happo diff on GovBanner is also a good sign that this refactor breaks nothing
  2. The new components are largely tested by GovBanner making use of them and GovBanner being tested, however each includes its own tests as well
  3. Run storybook and view the Components/Banner/Custom Banner story

Screenshots:

Custom banner storybook example (this one just changes some of the header text, but you can change anything! On the DOL project, we use the compositional structure to also allow us to internationalize all of the text and aria content with i18n!:

image

@brandonlenz brandonlenz self-assigned this Aug 8, 2022
@brandonlenz brandonlenz changed the title [feat] Add compositional Banner components feat: Add compositional Banner components Aug 8, 2022
Copy link
Contributor

@gidjin gidjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for updating this and the little refactoring for MediaBlockBody

Comment on lines +30 to +31
const getCopy = (language: Language, tld: TLD): GovBannerCopy => {
const lock = <BannerLockImage title="Lock" description="A locked padlock" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: I'm wondering if we need to think about internationalization support differently as we add more languages. This isn't something I'm thinking for this PR or anything just starting to wonder as I review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strongly agreed. Added a i18n label to capture what issues have occurred so far related to this topic i18n Relates to internationalization or localization standards for reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat] Make GovBanner more modular [feat] Update Gov banner copy
4 participants