USWDS - Alert and Site alert: Adjust left alignment calculations #5636
Conversation
- Rename $alert-wrapper-margin --> $site-alert-alignment - Change $theme-alert-icon-gap --> $alert-icon-gap
…t-icon-alignment
- File no longer needed
…t-icon-alignment
packages/usa-alert/src/test/test-patterns/test-alert-comparison.twig
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@mejiaj I believe I have addressed all your comments and made the updates requested in our discussion today. I did the following:
- Reverted the change that created additional variants for site alert. Site alert and alert will continue to have the same variants that they did in 3.8.2 and before.
- Updated the alignment methods so that it uses
paddingrather thanleftandmargin. This should improve clarity in the setting names and will make styling the components a little more intuitive. - Made no change regarding the role of
$theme-alert-padding-xin site alert. After our group discussion, we agreed that all other alert settings will affect site alert but not this one. This is because of the core horizontal alignment differences across components. As a next step, I will enhance the related documentation to make this clear. - Created a shared JSON file (
test-usa-alert-content.JSON) for alert test content and removed the content from the test twig files. - Confirmed that the template markup is different enough to keep them separate (but let me know if you disagree)
I think that is it. Let me know if you have any questions or want to walk through this in real time together.
- This prevents an error with build:html
There was a problem hiding this comment.
Note
I originally placed this file in the /test-patterns directory, but it caused an error when running npm run build:html. I moved it to a couple of different spots, but the only place it didn't cause an error is in this /content directory. If you see a better place for it, please let me know!
There was a problem hiding this comment.
@amyleadem I was able to see the new defaults and modify settings in your sandbox branch [al-test-5636-alert-align] without any issues. Thanks for creating that!
Settings I tested:
// _uswds-theme.scss
// Tested with varying combinations.
@use "uswds-core" with (
$theme-site-margins-width: 4,
$theme-alert-bar-width: 2,
$theme-alert-icon-size: 2,
$theme-alert-padding-x: 4,
$theme-alert-padding-y: 4,
);Added a few comments to improve readability and code re-use templates.
packages/usa-alert/src/test/test-patterns/test-alert-in-template.twig
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Default Settings
- At mobile, confirm that all alert and site alert variants have the same left and right alignment
- At desktop, confirm that site alert content shares left and right alignment with banner and footer
- At desktop, confirm that all alert variants share the same left alignment
Settings Adjustments
Set $theme-site-margins-width to 15 and 0
- At mobile, all alerts and site alerts share the same left and right alignment
Maybe I'm misunderstanding what I should be seeing from this test, but I see the site-alert alignment different from the alert alignment in mobile view? This screenshot is with theme-site-margins-width set to 0.
- At desktop, all site alerts share the same left and right alignment as the banner
- At desktop, all alerts share the same left and right alignment as each other
- The gap between the icon and the alert text does not change from default
Everything else looks good here!
Set $theme-alert-bar-width to 15 and 0
- The left colored bar adjusts to match the declared width
- Text content does not overlap with the colored bar
Everything looks good here!
Set $theme-alert-padding-x to 15 and 2. For each instance, confirm in the test stories that
- At desktop and mobile, the left and right alignment on the alerts adjusts to match the new setting value
- At desktop, all site alerts maintain the same left and right alignment as the banner
- At mobile, all site alerts share the same left and right alignment of 2.5 units
Everything looks good here!
Set $theme-alert-icon-size to 6 and 2
- The standard icon size adjusts to match the declared value (this should not affect the slim icon implementations)
- At all window widths, the text for alert and site alert components adjusts left alignment to match the new icon size. The gap between the icon and the text should not change, and the elements should not overlap.
Everything looks good here!
Set $theme-alert-padding-y to 6 and 0
- The vertical padding adjusts to match the declared value (this does not apply to the slim variant)
- All other elements align as expected
Everything looks good here!
Other
- Adjusting $theme-alert-padding-x adjusted the gap between the icon and text in 3.1.0. It no longer does that in this PR
Confirmed that the gap between icon and text in this PR does not change based on $theme-alert-padding-x setting
- The site alert in 3.1.0 does NOT have a bar on left side of the component. It does in develop and in this PR.
Confirmed that the bar on the left side of the component appears in this PR
|
@RachelCorsino If you look at the Test alert site alert comparison story, you should see that the two components align in mobile. Please let me know if you have any questions! |
|
@amyleadem - I see it working as expected in the Test alert site alert comparison story. Everything looks good from my tests - Approved! |
Summary
Fixed a bug that caused
$theme-site-margins-widthto adjust alignment inside the alert and site alert components.Important
At desktop widths, the site alert and alert components will no longer share left and right alignment. If users want alerts to align with site alert, they should use site alert markup.
Breaking change
Alignment on the alert and site alert components will likely shift from this change. Confirm that your implementation aligns as expected.
Related issue
Closes #5583
Related pull requests
Changelog PR
Preview link
Problem statement
When
$theme-site-margins-widthreceives custom values, it can cause the the content inside the alert component to overlap or become misaligned with the banner and other components. For example, setting$theme-site-margins-width: 1pxresults in the following:Setting the value to
$theme-site-margins-width:15` results in the following:And
$theme-site-margins-width:6`:Solution
This PR rebuilds the alignment in the alert and site alert components so that they consume settings values as expected.
Screenshots
Mobile
Desktop
Testing and review
Generally
Default state
With settings adjustments
Important
All alert component settings apply to site alert except
$theme-alert-padding-x. This is because the horizontal alignment of the site alert is fundamentally different from the alert component. We will update the documentation to clarify this in the changelog PR. The conversation about this decision is outlined in this comment thread$theme-site-margins-widthto 15 and 0. For each instance, confirm in the test stories that:$theme-alert-bar-widthto 15 and 0. For each instance, confirm in the test stories that:$theme-alert-padding-xto 15 and 2. For each instance, confirm in the test stories that:$theme-alert-icon-sizeto 6 and 2. For each instance, confirm in the test stories that:$theme-alert-padding-yto 6 and 0. For each instance, confirm in the test stories that:Compare to 3.1.0
Additionally, you can compare this PR with the alerts in the release-3.1.0 branch to see the components before the alignment adjustments in 3.2.0.
The main differences that I found between the 3.1.0 and this PR are:
$theme-alert-padding-xadjusted the gap between the icon and text in 3.1.0. It no longer does that in this PR, which I think is an improvement. (The gap is now consistent regardless of x padding size)