-
Notifications
You must be signed in to change notification settings - Fork 1.1k
USWDS - Align alert content with banner #4922
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
Conversation
@mixin site-alert-margins { | ||
&:before { | ||
left: units($theme-site-margins-mobile-width); | ||
@include at-media($theme-site-margins-breakpoint) { | ||
left: units($theme-site-margins-width); | ||
} | ||
} | ||
} | ||
|
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.
Removed because it was only referenced once in our system. Let me know if we need to preserve this mixin for project use, etc
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.
Both alert and site alert look good! Added some suggestions for code clarity.
|
||
.usa-alert__body { | ||
padding-left: $alert-slim-icon-size + (2 * $alert-icon-optical-padding); | ||
&.usa-alert--no-icon { |
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.
Does this mean that the default state of the alert includes an icon?
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.
The default usa-alert
and usa-site-alert
classes do not have an icon by default, but the default styles for status variants (usa-alert--emergency, usa-alert--info, etc) do have default icons. So the usa-alert--no-icon
class is useful if, for example, a user wants emergency styling without the icon.
packages/usa-alert/src/test/test-patterns/test-alert-comparison.json
Outdated
Show resolved
Hide resolved
packages/uswds-core/src/styles/mixins/helpers/alert-status-styles.scss
Outdated
Show resolved
Hide resolved
packages/uswds-core/src/styles/mixins/helpers/alert-status-styles.scss
Outdated
Show resolved
Hide resolved
packages/uswds-core/src/styles/mixins/helpers/alert-status-styles.scss
Outdated
Show resolved
Hide resolved
Thanks for the review @mejiaj. Let me know if you see anything else. |
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.
Great, thanks @amyleadem.
Tested the following
- Cross browser visual check (all screen sizes)
- Code style
- Ran
npm test
No issues found.
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.
Nice improvement!
when a site alert immediately follows a gov banner.
PR uswds#4922 added a top margin to the site alert, which looks wrong when a site alert immediately follows a gov banner.
USWDS - Site Alert - Repair top margin, broken by #4922
Summary
Updated the alignment of the alert and site-alert component content to visually match alignment with banner content.
Related issue
Closes #4642
Preview link
Preview link: Alert comparison test
Problem statement
Design elements like icons and text should align to make it easier for users to scan content. However, the alert, site-alert, and banner components all align along different vertical lines, which can cause visual disruption.
Solution
Left-aligning all items that use the
add-responsive-site-margins
mixin along the same vertical line will enable users to more easily scan content.Before
After
Major changes
Testing and review
To test:
Before opening this PR, make sure you’ve done whichever of these applies to you:
git pull origin [base branch]
to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch isdevelop
).npm run prettier:scss
to format code.npm test
and confirm that all tests pass.