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

Simplify font family settings #3949

Merged
merged 2 commits into from
Jul 20, 2023
Merged

Simplify font family settings #3949

merged 2 commits into from
Jul 20, 2023

Conversation

36degrees
Copy link
Contributor

Before we removed compatibility mode, the font-families file included multiple ‘settings’ (realistically, more like constants) that defined the font to be used depending on whether compatibility mode was enabled or not.

Now that we only have one possible default font stack, we can remove the unnecessary abstraction.

We also need to update the default value for the $govuk-include-default-font-face setting. Previously it checked whether the chosen font family was the default stack based on GDS Transport, but we can instead just check whether “GDS Transport” is in the font list. This also makes it more robust if somebody for example wants to update the setting to change the fallback fonts.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3949 July 11, 2023 16:48 Inactive
@36degrees 36degrees force-pushed the simplify-font-settings branch from 638a7b1 to 6b59673 Compare July 11, 2023 16:50
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3949 July 11, 2023 16:50 Inactive
@36degrees 36degrees force-pushed the simplify-font-settings branch from 6b59673 to e110288 Compare July 12, 2023 08:28
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3949 July 12, 2023 08:28 Inactive
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.

Happy to approve

Should we have deprecated the class first?

Probably want to add a CHANGELOG entry to add:

  1. Removed public variable $govuk-font-family-gds-transport
  2. Action to use variable $govuk-font-family instead

@36degrees
Copy link
Contributor Author

Should we have deprecated the class first?

Hmm, yeah, I think ideally we would have – it looks like the other font family 'settings' were deprecated at the point the new font was introduced back in 2019!

Not sure it's worth doing an extra 4.x release just for this, but if we do another before v5 maybe we should include this deprecation then?

Alternatively we hold off on this change until v6?

Probably want to add a CHANGELOG entry to add:

  1. Removed public variable $govuk-font-family-gds-transport
  2. Action to use variable $govuk-font-family instead

Yep, we also haven't mentioned the removal of the settings for the old version of the font which have already been removed.

A quick search of Google suggests that people are most often using $govuk-font-family-gds-transport to set the font on something, where really they should be using the govuk-font mixin instead. I'll try and write a changelog entry to nudge people towards that.

@colinrotherham
Copy link
Contributor

Thanks @36degrees

I keep forgetting about GitHub code search too:
https://github.com/search?q=%22%24govuk-font-family-gds-transport%22&type=code

Perhaps see if issues like #3918 will become another v4 release? Good to deprecate it before v5

Before we removed compatibility mode, the font-families file included multiple ‘settings’ (realistically, more like constants) that defined the font to be used depending on whether compatibility mode was enabled or not.

Now that we only have one possible default font stack, we can remove the unnecessary abstraction.

We also need to update the default value for the `$govuk-include-default-font-face` setting. Previously it checked whether the chosen font family was the default stack based on GDS Transport, but we can instead just check whether “GDS Transport” is in the font list. This also makes it more robust if somebody for example wants to update the setting to change the fallback fonts.
@36degrees 36degrees force-pushed the simplify-font-settings branch from e110288 to de9bd97 Compare July 19, 2023 16:26
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3949 July 19, 2023 16:26 Inactive
@36degrees
Copy link
Contributor Author

Rebased and added a changelog entry

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.

🎉

@36degrees 36degrees merged commit 4de004c into main Jul 20, 2023
@36degrees 36degrees deleted the simplify-font-settings branch July 20, 2023 08:45
@paulrobertlloyd
Copy link
Contributor

If there is another 4.x release to deprecate things, please don’t forget #3877

@romaricpascal romaricpascal mentioned this pull request Dec 8, 2023
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.

4 participants