Skip to content
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

Deprecate 14 on the type scale #4649

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Deprecate 14 on the type scale #4649

merged 3 commits into from
Feb 1, 2024

Conversation

owenatgov
Copy link
Contributor

What/Why

Deprecates 14 on the responsive typography scale along with the classes govuk-body-xs and govuk-!-font-size-14 as we're removing this option from the typography scale in 6.0.

Fixes #2786.

Thoughts

Deprecating a key in a map is tricky. The solution I've gone for here is that govuk-font-size looks for a deprecation key and a deprecation message in the responsive scale. If they're there, it outputs a warning and removes those keys from the font map in the mixin so that the deprecation keys don't break the loop. To get around this internally, I've added a _14 type scale point which replicates 14.

I've done this so that the deprecation is intrinsically tied to the scale value that we're removing. The downside is that users can override $govuk-typography-scale to get around the warning, however they could do that anyway with warning suppression.

The other options I explored are:

  • Throw a warning if $size == 14. This feels a bit fragile to me as it's just looking at the map name. There's a theoretical, very unlikely case where a user has a custom type scale map where one of the keys is 14 but the actual returned font sizes are higher than 14.
  • Throw a warning if the returned size from a given font-map is below 16px (and rem equivalent). This solution also infers that at 6.0 we'd throw an error if the same thing happened to block any user using font sizes below 16px regardless of custom type scales. Like the last option, this feels like it's disrupting the customisation of govuk-frontend and doesn't tie the actual scale value we're removing to the deprecation. I don't know how I feel about the error idea when we reach 6.0. I'm interested in views on this.

@owenatgov owenatgov requested a review from a team January 16, 2024 17:15
Copy link

github-actions bot commented Jan 16, 2024

📋 Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 112.45 KiB
dist/govuk-frontend-development.min.js 38.58 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 78.74 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 73.99 KiB
packages/govuk-frontend/dist/govuk/all.mjs 3.86 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 112.44 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 38.57 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.38 KiB

Modules

File Size
all.mjs 70.32 KiB
components/accordion/accordion.mjs 21.67 KiB
components/button/button.mjs 4.7 KiB
components/character-count/character-count.mjs 21.24 KiB
components/checkboxes/checkboxes.mjs 5.83 KiB
components/error-summary/error-summary.mjs 6.57 KiB
components/exit-this-page/exit-this-page.mjs 16.08 KiB
components/header/header.mjs 4.46 KiB
components/notification-banner/notification-banner.mjs 4.93 KiB
components/radios/radios.mjs 4.83 KiB
components/skip-link/skip-link.mjs 4.39 KiB
components/tabs/tabs.mjs 10.16 KiB

View stats and visualisations on the review app


Action run for abc3172

Copy link

github-actions bot commented Jan 16, 2024

Other changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/core/_typography.scss b/packages/govuk-frontend/dist/govuk/core/_typography.scss
index 11a58ebf9..5bc6cb4a0 100644
--- a/packages/govuk-frontend/dist/govuk/core/_typography.scss
+++ b/packages/govuk-frontend/dist/govuk/core/_typography.scss
@@ -128,14 +128,16 @@
     @extend %govuk-body-s;
   }
 
+  // @deprecated
   %govuk-body-xs {
     @include govuk-text-colour;
-    @include govuk-font($size: 14);
+    @include govuk-font($size: _14);
 
     margin-top: 0;
     @include govuk-responsive-margin(4, "bottom");
   }
 
+  // @deprecated
   .govuk-body-xs {
     @extend %govuk-body-xs;
   }
diff --git a/packages/govuk-frontend/dist/govuk/helpers/_typography.scss b/packages/govuk-frontend/dist/govuk/helpers/_typography.scss
index c0066778a..c73b94935 100644
--- a/packages/govuk-frontend/dist/govuk/helpers/_typography.scss
+++ b/packages/govuk-frontend/dist/govuk/helpers/_typography.scss
@@ -99,14 +99,14 @@
 
 /// Font size and line height helper
 ///
-/// @param {Number} $size - Point from the spacing scale (the size as it would
+/// @param {Number} $size - Point from the type scale (the size as it would
 ///   appear on tablet and above)
 /// @param {Number} $override-line-height [false] - Non responsive custom line
 ///   height. Omit to use the line height from the font map.
 /// @param {Boolean} $important [false] - Whether to mark declarations as
 ///   `!important`.
 ///
-/// @throw if `$size` is not a valid point from the spacing scale
+/// @throw if `$size` is not a valid point from the type scale
 ///
 /// @access public
 ///
@@ -144,24 +144,33 @@
 ///   )
 /// );
 ///
-/// @param {Number} $size - Point from the spacing scale (the size as it would
-///   appear on tablet and above)
+/// @param {Number | String} $size - Point from the type scale (the size as
+///   it would appear on tablet and above)
 /// @param {Number} $line-height [false] - Non responsive custom line
 ///   height. Omit to use the line height from the font map.
 /// @param {Boolean} $important [false] - Whether to mark declarations as
 ///   `!important`.
 ///
-/// @throw if `$size` is not a valid point from the spacing scale
+/// @throw if `$size` is not a valid point from the type scale
 ///
 /// @access public
 
 @mixin govuk-font-size($size, $line-height: false, $important: false) {
   @if not map-has-key($govuk-typography-scale, $size) {
-    @error "Unknown font size `#{$size}` - expected a point from the typography scale.";
+    @error "Unknown font size `#{$size}` - expected a point from the type scale.";
   }
 
   $font-map: map-get($govuk-typography-scale, $size);
 
+  // Check for a deprecation within the type scale
+  $deprecation: map-get($font-map, "deprecation");
+
+  @if $deprecation {
+    @include _warning(map-get($deprecation, "key"), map-get($deprecation, "message"));
+    // remove the deprecation map keys so they do not break the breakpoint loop
+    $font-map: map-remove($font-map, "deprecation");
+  }
+
   @each $breakpoint, $breakpoint-map in $font-map {
     $font-size: map-get($breakpoint-map, "font-size");
     $font-size-rem: govuk-px-to-rem($font-size);
@@ -203,14 +212,15 @@
 
 /// Font helper
 ///
-/// @param {Number | Boolean} $size Point from the spacing scale (the size as it
-///   would appear on tablet and above). Use `false` to avoid setting a size.
+/// @param {Number | Boolean | String} $size Point from the type scale (the
+///   size as it would appear on tablet and above). Use `false` to avoid setting
+///   a size.
 /// @param {String} $weight [regular] - Weight: `bold` or `regular`
 /// @param {Boolean} $tabular [false] - Whether to use tabular numbers or not
 /// @param {Number} $line-height [false] - Line-height, if overriding the
 ///   default
 ///
-/// @throw if `$size` is not a valid point from the spacing scale (or false)
+/// @throw if `$size` is not a valid point from the type scale (or false)
 ///
 /// @access public
 
diff --git a/packages/govuk-frontend/dist/govuk/overrides/_typography.scss b/packages/govuk-frontend/dist/govuk/overrides/_typography.scss
index 8859b7d18..63567cd68 100644
--- a/packages/govuk-frontend/dist/govuk/overrides/_typography.scss
+++ b/packages/govuk-frontend/dist/govuk/overrides/_typography.scss
@@ -3,9 +3,22 @@
 
   // Generate typography override classes for each responsive font map in the
   // typography scale eg .govuk-\!-font-size-80
-  @each $size in map-keys($govuk-typography-scale) {
-    .govuk-\!-font-size-#{$size} {
-      @include govuk-font-size($size, $important: true);
+  //
+  // govuk-!-font-size-14 is deprecated
+  @each $size, $font-map in $govuk-typography-scale {
+    // Suppress class if the map key is a string with an underscore eg: _14 to
+    // avoid classes like govuk-!-font-size-_14 getting generated
+    @if type-of($size) == "number" or str-slice(#{$size}, 1, 1) != "_" {
+      .govuk-\!-font-size-#{$size} {
+        $font-map: map-get($govuk-typography-scale, $size);
+
+        // Add underscore to deprecated typography scale keys
+        @if map-has-key($font-map, "deprecation") {
+          $size: _#{$size};
+        }
+
+        @include govuk-font-size($size, $important: true);
+      }
     }
   }
 
diff --git a/packages/govuk-frontend/dist/govuk/settings/_typography-responsive.scss b/packages/govuk-frontend/dist/govuk/settings/_typography-responsive.scss
index 730c2ce3e..bdb5972e8 100644
--- a/packages/govuk-frontend/dist/govuk/settings/_typography-responsive.scss
+++ b/packages/govuk-frontend/dist/govuk/settings/_typography-responsive.scss
@@ -140,6 +140,25 @@ $govuk-typography-scale: (
     )
   ),
   14: (
+    null: (
+      font-size: 12px,
+      line-height: 15px
+    ),
+    tablet: (
+      font-size: 14px,
+      line-height: 20px
+    ),
+    print: (
+      font-size: 12pt,
+      line-height: 1.2
+    ),
+    deprecation: (
+      key: "govuk-typography-scale-14",
+      message: "14 on the type scale is deprecated and will be removed as " +
+        "a possible option in the next major version."
+    )
+  ),
+  _14: (
     null: (
       font-size: 12px,
       line-height: 15px

Action run for abc3172

@owenatgov
Copy link
Contributor Author

Oh dear, that percy test is pretty damning. Will fix.

@owenatgov
Copy link
Contributor Author

Fixed 😎

Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a nice clever way to deprecate Sass map values

If everyone's happy with the approach, I've only got a few content/naming suggestions

CHANGELOG.md Outdated Show resolved Hide resolved
packages/govuk-frontend/src/govuk/helpers/_typography.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good Owen

Presume we need to suppress .govuk-!-font-size-_14 being output in the CSS?

The old (deprecated) class .govuk-!-font-size-14 is still there

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@owenatgov owenatgov changed the title Deprecate 14 on the typography scale Deprecate 14 on the type scale Feb 1, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@claireashworth
Copy link
Contributor

Just some minor tweak suggestions from me.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4649 February 1, 2024 15:09 Inactive
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.

Deprecate 14 on the font scale
4 participants