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

USWDS - Site Alert - Repair top margin, broken by #4922 #5550

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

lpsinger
Copy link
Contributor

@lpsinger lpsinger commented Oct 8, 2023

Summary

Removed the top margin from the site alert component.

PR #4922 added a top margin to the site alert, which looks wrong when a site alert immediately follows a gov banner.

Breaking change

This is not a breaking change.

Related PR

Changelog PR

Preview link

This affects https://test.gcn.nasa.gov.

USWDS 3.1.0

uswds-3 1 0

USWDS 3.6.1, before this patch

before

After this patch

after

Problem statement

The site alert should not have any top margin. It didn't before USWDS 3.6.1. The margin looks like a glitch when a site alert immediately follows a gov banner.

Solution

Move the top margin back from the mixin to _usa-alert.scss so that applies to alerts but not site alerts.

Testing and review

I tested this locally to produce the screenshots above.

Dependency updates

None.

PR uswds#4922 added a top margin to the site alert, which looks wrong
when a site alert immediately follows a gov banner.
@mejiaj mejiaj requested a review from amyleadem October 11, 2023 19:19
@thisisdano thisisdano added the Role: Dev Development/engineering skills needed label Oct 12, 2023
@thisisdano thisisdano added this to the uswds 3.7.0 milestone Oct 12, 2023
@thisisdano
Copy link
Member

Great patch. We're going to work to get this into 3.7.0.

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Thank you for this fix! I confirmed that this PR removes the top margin from the site alert component and preserves the top margin on the alert component.

Before

Site alert
image

Alert
image

After

Site alert
image

Alert
image

Copy link
Contributor

@mahoneycm mahoneycm 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 taking care of this @lpsinger 👏

  • Site alert no longer has a margin between banner and alert
  • Standard alerts are unaffected by this change
  • No visual regressions or build errors

Copy link
Member

@thisisdano thisisdano left a comment

Choose a reason for hiding this comment

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

Thank you!

@thisisdano thisisdano merged commit 5e3dd52 into uswds:develop Nov 2, 2023
@lpsinger lpsinger deleted the usa-site-alert-fix-top-margin branch November 2, 2023 18:36
@thisisdano thisisdano mentioned this pull request Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: Site Alert Role: Dev Development/engineering skills needed
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants