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

Add "centered" banner component variant #429

Merged
merged 2 commits into from
Mar 19, 2024
Merged

Add "centered" banner component variant #429

merged 2 commits into from
Mar 19, 2024

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 19, 2024

🛠 Summary of changes

Formalizes the "centered" variant of the banner component used in the IdP, and adds documentation for banner component usage.

This also includes revisions and recommendations that build upon the default USWDS documentation, slated for upstream pull requests:

  • Add width and height attributes to all images (related blog post, avoids common automated scanner issues)
  • Use us_flag.svg (vector) instead of us_flag_small.png (raster) for improved scalability, smaller file size
  • Replaces inline SVG element with reference to icon.svg as <img /> element

📜 Testing Plan

  1. npm start
  2. Visit http://localhost:4000/banner/
  3. Verify banner appears as expected, both "Default" and "Centered" variants, including recommended markup

👀 Screenshots

localhost_4000_banner_(Computer Standard)

## Centered

{% capture example %}
<section
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a lot of repeated stuff for just the id and class to be different, do we ever consider making these into partials w/ variables?

2c2
<   class="usa-banner"
---
>   class="usa-banner usa-banner--centered"
28c28
<           aria-controls="gov-banner-default"
---
>           aria-controls="gov-banner-centered"
36c36
<       id="gov-banner-default"
---
>       id="gov-banner-centered"

Copy link
Member Author

@aduth aduth Mar 19, 2024

Choose a reason for hiding this comment

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

Yes! It was made difficult to propose with how I split up the pull requests, but part of what I'm hoping with #433 is to just {% include banner.html %} for these component examples and have some variable (or auto-incrementing number) to create unique IDs.

Reflected in that PR message:

This also opens the possibility of internally reusing "includes" for header with documented usage for guaranteed one-to-one documentation and usage

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try to arrange merge order of the pull requests so that I can rebase to include this improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rebased and updated to reuse banner.html in 4b3fcca.

@aduth aduth force-pushed the aduth-doc-banner branch from 9131538 to 4b3fcca Compare March 19, 2024 20:00
@aduth
Copy link
Member Author

aduth commented Mar 19, 2024

Visual regression failure expected for the added navigation link.

image

@aduth aduth merged commit 9a1b072 into main Mar 19, 2024
3 of 4 checks passed
@aduth aduth deleted the aduth-doc-banner branch March 19, 2024 20:03
@aduth aduth mentioned this pull request Mar 22, 2024
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.

3 participants