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

Remove unnecessary uses of govuk-font #3516

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Remove unnecessary uses of govuk-font #3516

merged 1 commit into from
Feb 7, 2024

Conversation

owenatgov
Copy link
Contributor

@owenatgov owenatgov commented Feb 2, 2024

What/Why

Removes uses of govuk-font which are unnecessary, typically because typographic elements such as the font family are cascading from elsewhere and only a font size change may be necessary.

Similar to alphagov/govuk-frontend#4267

Notes

Impact on CSS output is fairly minimal. Running build against this branch vs main shows that this branch's CSS is 1 kilobyte 2 kilobyets lighter. I would still argue this is a useful change on the basis that it's ensuring we don't duplicate CSS unnecessarily.

Several removes of govuk-font are because that element's typography-common is being defined by app-prose and cascading down. In some cases this is convenient but not intentional eg: app-tabs__item only needing govuk-typography-responsive because app-prose is applying typography to lists, which app-tabs are semantically but not necessarily typographically. There have been unofficial chats about replacing app-prose with marked post-processing to apply govuk classes to typographic elements in content. If this happens then we would want to put govuk-font back on these elements.

@owenatgov owenatgov requested a review from a team February 2, 2024 10:43
Copy link

netlify bot commented Feb 2, 2024

You can preview this change here:

Name Link
🔨 Latest commit ff0c3a2
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/65c3501373638200089218fa
😎 Deploy Preview https://deploy-preview-3516--govuk-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines -24 to -33
@include govuk-font($size: 24);
@include govuk-responsive-padding(6);

display: block;
margin: 0;

color: govuk-colour("white");
background-color: govuk-colour("blue");

text-align: center;
Copy link
Contributor

Choose a reason for hiding this comment

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

Confirming that these styles were duplicated in grid-annotate.scss 👍

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.

Always good to see 🙌

Added a few comments, maybe that 1 kilobyte could be more?

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.

Looks good to me, approving with one comment

The code text looks a bit "thick" now we've lost these two:

-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale;

Shall we add it to the pre, code block here?

pre,
code {
font-family: ui-monospace, Menlo, "Cascadia Mono", "Segoe UI Mono", Consolas, "Liberation Mono", monospace;
}

@paulrobertlloyd
Copy link
Contributor

There have been unofficial chats about replacing app-prose with marked post-processing to apply govuk classes to typographic elements in content.

In case you’re unaware, here are plugins for 2 popular Markdown parsers that do this post-processing:

@owenatgov
Copy link
Contributor Author

@colinrotherham Resolved! I forgot you could pass a custom $font-family with govuk-typography-common.

@owenatgov owenatgov merged commit 9ae6c3e into main Feb 7, 2024
13 checks passed
@owenatgov owenatgov deleted the govuk-font-cleanup branch February 7, 2024 09:43
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.

3 participants