-
Notifications
You must be signed in to change notification settings - Fork 934
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
Alert: Remove invalid and redundant alert height style #5187
Conversation
- This produces invalid styles because `$theme-alert-icon-size` is a unitless number (e.g. `2`) and should be wrapped with the `unit` function - This is redundant anyways because the `height` is already assigned by the inclusion of `add-color-icon` mixin above using the `height` value from `$this-ico-object`
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.
Confirming this effectively removes the redundant height definition from the alert icon!
The icon height is still defined through $theme-alert-icon-size via $this-icon-object directly above in the CSS. Removing the height definition from $theme-icon-object also does not use the height defined within the &::before selector so we can safely remove.
|
This issue might not actually be so harmless in all cases, since some CSS post-processors may try to correct the style by adding a missing unit. For example, Lightning CSS will change the $ node -p "require('lightningcss').transform({ code: Buffer.from('body { height: 4; }') }).code.toString()"
body {
height: 4px;
}Edit: See example impact at 18F/identity-idp#8093 (comment) |
|
Created change log PR for site: uswds/uswds-site#2094 |
Summary
Removes an unnecessary assignment of
heightstyling in theadd-alert-iconmixin.$theme-alert-icon-sizeis a unitless number (e.g.2) and should be wrapped with theunitfunctionheightis already assigned by the inclusion ofadd-color-iconmixin above using theheightvalue from$this-icon-objectTesting and review
Observe the issue by inspecting the
::beforepseudo-element of.usa-alert__bodyin the Alert component preview page.The changes proposed in this pull request will cause the second (invalid) highlighted
heightstyle to be removed.