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

Ensure govuk-font-size() handles string keys #4713

Merged
merged 4 commits into from
Feb 2, 2024

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Feb 1, 2024

Quick change to split out some code review ideas in #4649 (comment)

Add tests for "T-Shirt Sizing" type scale

This PR adds new tests for services that extend $govuk-typography-scale with string keys:

@include govuk-font-size($size: "small") //
@include govuk-font-size($size: "medium") //
@include govuk-font-size($size: "large") //

Ensuring that we treat sizes 14, “14”, “_14” and named “x-large” size keys equally

Remove duplicate font declaration for "internal use" point 14

This PR automatically maps "_14" to 14 as part of govuk-font-size() by removing the underscore:

@include govuk-font-size($size: "_14") //
@include govuk-font-size($size: "14") //
@include govuk-font-size($size: 14) //

We can now remove the duplicated _14 declaration:

_14: (
null: (
font-size: 12px,
line-height: 15px
),
tablet: (
font-size: 14px,
line-height: 20px
),
print: (
font-size: 12pt,
line-height: 1.2
)
)

Copy link

github-actions bot commented Feb 1, 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 ffbfd86

Copy link

github-actions bot commented Feb 1, 2024

Other changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/helpers/_typography.scss b/packages/govuk-frontend/dist/govuk/helpers/_typography.scss
index c73b94935..44e4167f1 100644
--- a/packages/govuk-frontend/dist/govuk/helpers/_typography.scss
+++ b/packages/govuk-frontend/dist/govuk/helpers/_typography.scss
@@ -156,17 +156,41 @@
 /// @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 type scale.";
+  // Flag font sizes that start with underscores so we can suppress warnings on
+  // deprecated sizes used internally, for example `govuk-font($size: "_14")`
+  $size-internal-use-only: str-slice(#{$size}, 1, 1) == "_";
+
+  // Remove underscore from font sizes flagged for internal use
+  @if $size-internal-use-only {
+    $size: str-slice(#{$size}, 2);
   }
 
+  // Check for a font map exactly matching the given size
   $font-map: map-get($govuk-typography-scale, $size);
 
+  // No match? Try with string type (e.g. $size: "16" not 16)
+  @if not $font-map {
+    @each $font-size in map-keys($govuk-typography-scale) {
+      @if not $font-map and #{$font-size} == #{$size} {
+        $font-map: map-get($govuk-typography-scale, $font-size);
+      }
+    }
+  }
+
+  // Still no match? Throw error
+  @if not $font-map {
+    @error "Unknown font size `#{$size}` - expected a point from the type scale.";
+  }
+
   // 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"));
+    // Warn on deprecated font sizes unless flagged for internal use
+    @if not $size-internal-use-only {
+      @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");
   }
diff --git a/packages/govuk-frontend/dist/govuk/overrides/_typography.scss b/packages/govuk-frontend/dist/govuk/overrides/_typography.scss
index 63567cd68..8ff1d7a7e 100644
--- a/packages/govuk-frontend/dist/govuk/overrides/_typography.scss
+++ b/packages/govuk-frontend/dist/govuk/overrides/_typography.scss
@@ -6,19 +6,15 @@
   //
   // 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);
+    .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);
+      // 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 bdb5972e8..0daa9673f 100644
--- a/packages/govuk-frontend/dist/govuk/settings/_typography-responsive.scss
+++ b/packages/govuk-frontend/dist/govuk/settings/_typography-responsive.scss
@@ -157,20 +157,6 @@ $govuk-typography-scale: (
       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
-    ),
-    tablet: (
-      font-size: 14px,
-      line-height: 20px
-    ),
-    print: (
-      font-size: 12pt,
-      line-height: 1.2
-    )
   )
 ) !default;
 

Action run for ffbfd86

Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

I love this, thanks Colin. I've left one comment but approving anyway as it's non-critical.

I think this deserves a changelog entry but it doesn't need a big one. I reckon either under fixes or you can add it to the 14 deprecation changelog entry ie: "this change was done in [original PR] and [this PR]".

packages/govuk-frontend/src/govuk/helpers/_typography.scss Outdated Show resolved Hide resolved
This lets us treat 14, “14”, “_14” and named “x-large” size keys equally
@colinrotherham
Copy link
Contributor Author

Thanks @owenatgov

I think this deserves a changelog entry but it doesn't need a big one. I reckon either under fixes or you can add it to the 14 deprecation changelog entry ie: "this change was done in [original PR] and [this PR]".

Updated with your suggestion ✅

colinrotherham and others added 3 commits February 2, 2024 12:09
Clearly explain why we’re flagging sizes that start with an underscore and what we’re going to do about it
We can remove the underscore in `_14` → `14` so the deprecated font declaration map continue to work without duplication
Co-authored-by: Owen Jones <owen.jones@digital.cabinet-office.gov.uk>
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4713 February 2, 2024 12:12 Inactive
@colinrotherham
Copy link
Contributor Author

@owenatgov Also realised, we no longer need to suppress .govuk-!-font-size-_14 output

// Suppress class if the map key is a string with an underscore eg: _14 to
// avoid classes like govuk-!-font-size-_14 getting generated

Updated in 220a4c0 where _14 is removed 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants