cfpb / design-system

CFPB's work-in-progress design system
https://cfpb.github.io/design-system
Creative Commons Zero v1.0 Universal
42 stars 13 forks source link

Refactor heading mixins #2055

Closed anselmbradford closed 1 month ago

anselmbradford commented 2 months ago

We have widely used heading mixins for setting heading sizes. The heading-1-heading-6 mixins set font and line height sizing, and add bottom margin. The h1-h6 mixins extend the heading- mixins and add top margin, and for h4 and up, add responsive styling for mobile sizes.

The heading mixins are either used as the standalone headings, or as the extended responsive version. In several uses, the margin of the heading is overridden. This is done by calling the mixin and then adding a separate margin property below the mixin to override the mixin. Additionally, rarely the text weight also gets overridden (e.g. on the h2 on the homepage 50-50 hero).

Sass has deprecated placing declarations below mixins (see https://sass-lang.com/documentation/breaking-changes/mixed-decls/), to align with the direction the CSS standard is going. We can still do this by wrapping the declaration in & { … }, but ultimately this generates a redundant selector block, so it's probably best to refactor things so that the mixins can be placed last.

This PR merges the h1-h6 mixins into the heading-1-heading-6 mixins so there is only one set of heading mixins to be dealing with. It makes these mixins take several arguments to remove the top or bottom margin, specify if its responsive or not (if it's heading-4 or larger), and whether the font-weight can be adjusted (if it's heading-2-heading-4—this is based on real-world usage of this override).

Changes

Testing

  1. Check the changed components on their respective PR preview pages.

At minimum @include h1-@include h6 should map to @include heading-1-@include heading-6.

@include heading-1-@include heading-4, should map to @include heading-1($is-responsive: false)-@include heading-4($is-responsive: false).

Heading 5 and 6 don't have responsive styles, so @include heading-5/@include heading-6 should be unchanged.

Notes

netlify[bot] commented 2 months ago

Thanks for the improvements! Browse a preview of your changes using the link below.

Name Link
Latest commit 6f421a6ef6ea17c076fe34db23bdfe855ee71cc4
Latest deploy log https://app.netlify.com/sites/cfpb-design-system/deploys/66ed9bccc5babe00084a151d
Deploy Preview https://deploy-preview-2055--cfpb-design-system.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.