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

Refactor tabular number activation into their own mixin #4307

Merged
merged 2 commits into from
Jan 5, 2024

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Oct 5, 2023

Splits the code that activates tabular numbers out of the govuk-font mixin into its own standalone mixin. Resolves #3778.

Warning

This PR is for future v5.x work. It should not be merged until after v5.0 has been released.

Changes

  • Creates a new mixin, govuk-font-tabular-numbers, to contain the existing CSS rules.
    • This name of the mixin is intended to be 'self explanatory', insomuch as it closely matches the CSS properties and values it's setting, similar to my suggestion for the font-weight mixins in this comment.
  • Simplifies the CSS rules for enabling tabular numbers.
    • Removes the use of font-feature-settings to define tabular numbers. This was only still needed for IE 11 support, and as it's only a stylistic flag, IE having proportional figures seems an acceptable fallback.
    • I'm not sure exactly why we were setting font-feature-settings: normal in the @supports query. This will override any customisations that a user may have made (see the thoughts below), and resetting the value doesn't do anything for our use case here. I think we can remove both it and the @supports query without causing any issues.
  • Changes govuk-font to call the new mixin instead.
  • Updates all instances where govuk-font was being called solely to activate tabular numbers to call the new mixin instead.

Thoughts

  • I've opted to keep and not deprecate govuk-font's $tabular parameter. I think it's Mostly Harmless and am inclined to leave it be unless a compelling reason to deprecate it is presented.
  • I've made this new mixin accept a parameter that sets !important to be consistent with the others and to potentially facilitiate a future utility or override class, which could help implement Help people use tabular numbers govuk-design-system#1233.
  • I'm not a fan offont-feature-settings. It's a very 'broad' property that is prone to overreach and the way we were using it would override or will be overridden by any other use of font-feature-settings that a user may set. As we now only had font-feature-settings for IE 11 support, it's not functionally critical in any way, and as we don't make any guarantee that IE will render identically, I've removed the property entirely.

@querkmachine querkmachine requested a review from a team October 5, 2023 18:42
@querkmachine querkmachine self-assigned this Oct 5, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4307 October 5, 2023 18:42 Inactive
@github-actions
Copy link

github-actions bot commented Oct 5, 2023

📋 Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 112.81 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.8 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 4deafca

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4307 October 5, 2023 19:16 Inactive
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.

This is looking sharp. Here are some thoughts off of your thoughts:

  • I think we can keep $tabular on govuk-font for instances where someone wants all the typography things at once.
  • The naming is a tricky one. I'd like to do some data gathering at the end of the typography work to see what names the community would prefer, however it feels to me like we're starting to move towards govuk-font-x rather than govuk-typography-x. I'm fine with us calling it something like govuk-font-variant-tabular.
  • Looking at the original code again, I read it as font-variant-numeric being the priority with font-feature-settings being the backup, so my instinct is to sack it. I feel even more confident cross referencing our 5.0 browser support grading against support for font-variant-numeric.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4307 October 6, 2023 15:40 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4307 October 19, 2023 11:23 Inactive
@querkmachine querkmachine requested a review from a team October 19, 2023 11:40
@querkmachine querkmachine marked this pull request as ready for review October 19, 2023 11:40
@querkmachine querkmachine changed the title Refactor tabular number activation into their own mixin [DO NOT MERGE] Refactor tabular number activation into their own mixin Oct 19, 2023
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.

One final comment. This is basically good to go as far as I'm concerned.

Would you like an approval tick now, pending my last comment, or once 5.0 is out?

@@ -62,6 +62,21 @@
font-weight: $govuk-font-weight-bold if($important, !important, null);
}

/// Tabular number helper
///
/// Using font-feature-settings is generally discouraged, but unfortunately
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this description is relevant anymore. I still think there's worth including some content explaining this is for tabular number readability.

Copy link
Member Author

@querkmachine querkmachine Oct 19, 2023

Choose a reason for hiding this comment

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

D'oh! Replaced it with a little explainer on why tabular numbers exist and what they're good for.

I think final approval can probably wait? It helps avoid an accidental merge, and there may also be some rebasing that happens in future that needs checking over closer to the time.

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

I think this broadly makes sense, but wondering if we should simplify the name slightly and just call it govuk-tabular-numbers?

I think it's obvious from what it does that it relates to fonts, and it feels a tad more memorable / guessable?

@querkmachine querkmachine changed the title [DO NOT MERGE] Refactor tabular number activation into their own mixin Refactor tabular number activation into their own mixin Dec 15, 2023
@querkmachine
Copy link
Member Author

@36degrees As per Owen's comment and [https://github.com//issues/4243](the naming issue), I think we're kinda leaning towards typography-related mixins being namespaced under govuk-font- to match how most of the CSS properties they're setting begin with font-.

I don't much mind if it's something like govuk-font-tabular-numbers though!

Copy link

github-actions bot commented Dec 15, 2023

Stylesheets changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
index 403e85fb3..4d94061f9 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
@@ -2318,28 +2318,11 @@
 }
 
 .govuk-character-count__message {
-    font-family: GDS Transport, arial, sans-serif;
-    -webkit-font-smoothing: antialiased;
-    -moz-osx-font-smoothing: grayscale;
-    font-feature-settings: "tnum" 1;
-    font-weight: 400;
+    font-variant-numeric: tabular-nums;
     margin-top: 0;
     margin-bottom: 0
 }
 
-@media print {
-    .govuk-character-count__message {
-        font-family: sans-serif
-    }
-}
-
-@supports (font-variant-numeric:tabular-nums) {
-    .govuk-character-count__message {
-        font-feature-settings: normal;
-        font-variant-numeric: tabular-nums
-    }
-}
-
 .govuk-character-count__message:after {
     content: "​"
 }
@@ -2805,27 +2788,10 @@ screen and (forced-colors:active) {
 }
 
 .govuk-input--extra-letter-spacing {
-    font-family: GDS Transport, arial, sans-serif;
-    -webkit-font-smoothing: antialiased;
-    -moz-osx-font-smoothing: grayscale;
-    font-feature-settings: "tnum" 1;
-    font-weight: 400;
+    font-variant-numeric: tabular-nums;
     letter-spacing: .05em
 }
 
-@media print {
-    .govuk-input--extra-letter-spacing {
-        font-family: sans-serif
-    }
-}
-
-@supports (font-variant-numeric:tabular-nums) {
-    .govuk-input--extra-letter-spacing {
-        font-feature-settings: normal;
-        font-variant-numeric: tabular-nums
-    }
-}
-
 .govuk-input--width-30 {
     max-width: 29.5em
 }
@@ -5684,24 +5650,7 @@ screen and (-ms-high-contrast:active) {
 }
 
 .govuk-table__cell--numeric {
-    font-family: GDS Transport, arial, sans-serif;
-    -webkit-font-smoothing: antialiased;
-    -moz-osx-font-smoothing: grayscale;
-    font-feature-settings: "tnum" 1;
-    font-weight: 400
-}
-
-@media print {
-    .govuk-table__cell--numeric {
-        font-family: sans-serif
-    }
-}
-
-@supports (font-variant-numeric:tabular-nums) {
-    .govuk-table__cell--numeric {
-        font-feature-settings: normal;
-        font-variant-numeric: tabular-nums
-    }
+    font-variant-numeric: tabular-nums
 }
 
 .govuk-table__cell--numeric,

Action run for 4deafca

Copy link

github-actions bot commented Dec 15, 2023

Other changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/components/character-count/_index.scss b/packages/govuk-frontend/dist/govuk/components/character-count/_index.scss
index cec8b615e..858b7735b 100644
--- a/packages/govuk-frontend/dist/govuk/components/character-count/_index.scss
+++ b/packages/govuk-frontend/dist/govuk/components/character-count/_index.scss
@@ -14,7 +14,7 @@
   }
 
   .govuk-character-count__message {
-    @include govuk-font($size: false, $tabular: true);
+    @include govuk-font-tabular-numbers;
     margin-top: 0;
     margin-bottom: 0;
 
diff --git a/packages/govuk-frontend/dist/govuk/components/input/_index.scss b/packages/govuk-frontend/dist/govuk/components/input/_index.scss
index b236edf84..694a0d214 100644
--- a/packages/govuk-frontend/dist/govuk/components/input/_index.scss
+++ b/packages/govuk-frontend/dist/govuk/components/input/_index.scss
@@ -58,7 +58,7 @@
   }
 
   .govuk-input--extra-letter-spacing {
-    @include govuk-font(false, $tabular: true);
+    @include govuk-font-tabular-numbers;
     letter-spacing: 0.05em;
   }
 
diff --git a/packages/govuk-frontend/dist/govuk/components/table/_index.scss b/packages/govuk-frontend/dist/govuk/components/table/_index.scss
index d27b2060b..c847ba233 100644
--- a/packages/govuk-frontend/dist/govuk/components/table/_index.scss
+++ b/packages/govuk-frontend/dist/govuk/components/table/_index.scss
@@ -22,7 +22,7 @@
   }
 
   .govuk-table__cell--numeric {
-    @include govuk-font($size: false, $tabular: true);
+    @include govuk-font-tabular-numbers;
   }
 
   .govuk-table__header--numeric,
diff --git a/packages/govuk-frontend/dist/govuk/helpers/_typography.scss b/packages/govuk-frontend/dist/govuk/helpers/_typography.scss
index cb60a0123..c0066778a 100644
--- a/packages/govuk-frontend/dist/govuk/helpers/_typography.scss
+++ b/packages/govuk-frontend/dist/govuk/helpers/_typography.scss
@@ -62,6 +62,23 @@
   font-weight: $govuk-font-weight-bold if($important, !important, null);
 }
 
+/// Tabular number helper
+///
+/// Switches numerical glyphs (0–9) to use alternative forms with a
+/// monospaced bounding box. This ensures that columns of numbers, such
+/// as those in tables, remain horizontally aligned with one another.
+/// This also has the useful side effect of making numbers more legible
+/// in some situations, such as reference codes, as the numbers are more
+/// distinct and visually separated from one another.
+///
+/// @param {Boolean} $important [false] - Whether to mark declarations as
+///   `!important`. Generally Used to create override classes.
+/// @access public
+
+@mixin govuk-font-tabular-numbers($important: false) {
+  font-variant-numeric: tabular-nums if($important, !important, null);
+}
+
 /// Convert line-heights specified in pixels into a relative value, unless
 /// they are already unit-less (and thus already treated as relative values)
 /// or the units do not match the units used for the font size.
@@ -201,12 +218,7 @@
   @include govuk-typography-common;
 
   @if $tabular {
-    font-feature-settings: "tnum" 1;
-
-    @supports (font-variant-numeric: tabular-nums) {
-      font-feature-settings: normal;
-      font-variant-numeric: tabular-nums;
-    }
+    @include govuk-font-tabular-numbers;
   }
 
   @if $weight == regular {

Action run for 4deafca

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.

Approving for that diff! #4307 (comment) 🙌

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.

This just needs a changelog entry and we're in the clear

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4307 January 5, 2024 10:41 Inactive
@querkmachine
Copy link
Member Author

@owenatgov Added a changelog entry. I wasn't totally sure how to describe the change or why people should use it over govuk-font. I opted to describe it as a new feature that can be used to slightly optimise CSS file sizes, but there might be a better take on it.

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 think the way you've described the change works well! Efficiency and neatness is what it's about really. Double approved!

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.

Split tabular numbers functionality out from the govuk-font mixin
5 participants