-
Notifications
You must be signed in to change notification settings - Fork 340
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
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for ffbfd86 |
Other changes to npm packagediff --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 |
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.
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]".
This lets us treat 14, “14”, “_14” and named “x-large” size keys equally
f80e7d0
to
410d74c
Compare
Thanks @owenatgov
Updated with your suggestion ✅ |
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>
4fe469f
to
ffbfd86
Compare
@owenatgov Also realised, we no longer need to suppress // 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 |
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: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"
to14
as part ofgovuk-font-size()
by removing the underscore:We can now remove the duplicated
_14
declaration:govuk-frontend/packages/govuk-frontend/src/govuk/settings/_typography-responsive.scss
Lines 161 to 174 in eb3c18d