Skip to content

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Aug 18, 2022

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

image

After

image

Major changes

  1. Removed usa-site-alert's dependency on usa-alert. This was done by consolidating all shared styles in uswds-core mixins.
  2. Visually aligned the content in both components with the content in usa-banner.

Testing and review

To test:

  1. Confirm that alert variant styles match existing display. Note: The alignment is expected to be different, but colors, typography, padding, etc should be the same.
  2. Confirm that styles left-align with other components like banner and footer
  3. Confirm that alignment looks good in multiple browsers at multiple widths.
  4. Confirm that you can disable all styles in usa-alert and still have proper display of usa-site-alert (and vice versa)

Before opening this PR, make sure you’ve done whichever of these applies to you:

  • Run git pull origin [base branch] to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch is develop).
  • Run npm run prettier:scss to format code.
  • Run npm test and confirm that all tests pass.

Comment on lines -9 to -17
@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);
}
}
}

Copy link
Contributor Author

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

@amyleadem amyleadem marked this pull request as ready for review August 25, 2022 22:29
@amyleadem amyleadem requested a review from mejiaj August 25, 2022 22:29
Copy link
Contributor

@mejiaj mejiaj left a 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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@amyleadem amyleadem requested a review from mejiaj August 29, 2022 17:50
@amyleadem
Copy link
Contributor Author

Thanks for the review @mejiaj. Let me know if you see anything else.

Copy link
Contributor

@mejiaj mejiaj left a 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.

@mejiaj mejiaj requested a review from thisisdano September 1, 2022 13:59
Copy link
Contributor

@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.

Nice improvement!

@thisisdano thisisdano merged commit 8f617a4 into develop Sep 26, 2022
@thisisdano thisisdano deleted the al-alert-icon-alignment branch September 26, 2022 15:43
This was referenced Oct 19, 2022
lpsinger added a commit to lpsinger/uswds that referenced this pull request Oct 8, 2023
when a site alert immediately follows a gov banner.
lpsinger added a commit to lpsinger/uswds that referenced this pull request Oct 8, 2023
PR uswds#4922 added a top margin to the site alert, which looks wrong
when a site alert immediately follows a gov banner.
thisisdano added a commit that referenced this pull request Nov 2, 2023
USWDS - Site Alert - Repair top margin, broken by #4922
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.

Design: Icons and text don't align in the banner and alerts
3 participants