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 duplicated use of govuk-font #4267

Merged
merged 3 commits into from
Oct 2, 2023

Conversation

owenatgov
Copy link
Contributor

@owenatgov owenatgov commented Sep 27, 2023

What/Why

Remove instances of when our components are using govuk-font more than once when they don't need to.

Fixes #4242. More details in the issue.

Notes

At time of writing, building main locally produces minified CSS of 123kb and a CSS map of 408kb. Building this branch locally produces minified CSS of 117kb and a CSS map of 397kb.

The only 2 components that still have more than one use of govuk-font are the header and input component. Both of these are because of user agent styles that override the inherited font size, weight and font family. Specifically the button element that govuk-header__menu-button is using and the input for govuk-input.

This change makes the following classes redundant:

  • govuk-panel__body
  • govuk-table__caption--s

Should we deprecate these classes?

This change additionally adds our default text colour to govuk-summary-card which was missed when we initially launched this.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4267 September 27, 2023 16:17 Inactive
@owenatgov owenatgov requested a review from a team September 27, 2023 16:25
@github-actions
Copy link

github-actions bot commented Sep 27, 2023

📋 Stats

File sizes

File Size
dist/govuk-frontend-4.6.0.min.css 118.07 KiB
dist/govuk-frontend-4.6.0.min.js 47.1 KiB
dist/govuk-frontend-ie8-4.6.0.min.css 79.27 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 77.65 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 72.94 KiB
packages/govuk-frontend/dist/govuk/all.mjs 3.84 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 113.99 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 37.89 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.83 KiB

Modules

File Size
all.mjs 69.04 KiB
components/accordion/accordion.mjs 21.91 KiB
components/button/button.mjs 4.71 KiB
components/character-count/character-count.mjs 22.03 KiB
components/checkboxes/checkboxes.mjs 5.69 KiB
components/error-summary/error-summary.mjs 6.01 KiB
components/exit-this-page/exit-this-page.mjs 16.61 KiB
components/header/header.mjs 3.84 KiB
components/notification-banner/notification-banner.mjs 4.55 KiB
components/radios/radios.mjs 4.68 KiB
components/skip-link/skip-link.mjs 3.74 KiB
components/tabs/tabs.mjs 9.32 KiB

View stats and visualisations on the review app


Action run for 32a6181

@owenatgov owenatgov force-pushed the remove-improper-use-of-govuk-font branch from 539c64d to 45807e2 Compare September 27, 2023 16:33
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4267 September 27, 2023 16:33 Inactive
@owenatgov owenatgov marked this pull request as ready for review September 28, 2023 13:51
@owenatgov owenatgov force-pushed the remove-improper-use-of-govuk-font branch from 45807e2 to 158c944 Compare September 28, 2023 13:55
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4267 September 28, 2023 13:55 Inactive
@owenatgov owenatgov force-pushed the remove-improper-use-of-govuk-font branch from 158c944 to 16d0085 Compare September 29, 2023 10:05
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4267 September 29, 2023 10:05 Inactive
Comment on lines +38 to +43
.govuk-fieldset__legend--xl,
.govuk-fieldset__legend--l,
.govuk-fieldset__legend--m {
@include govuk-typography-weight-bold;
margin-bottom: govuk-spacing(3);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@owenatgov owenatgov force-pushed the remove-improper-use-of-govuk-font branch from 16d0085 to e3118ff Compare September 29, 2023 15:09
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4267 September 29, 2023 15:09 Inactive
This isn't necessary as we already have heading style modifiers for table captions. This does mean it's inconsistent with our docs that say they should correspond with heading typography classes. Suggest we should solve this holistically down the line.
@owenatgov owenatgov force-pushed the remove-improper-use-of-govuk-font branch from e3118ff to 32a6181 Compare September 29, 2023 15:15
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4267 September 29, 2023 15:16 Inactive
@colinrotherham
Copy link
Contributor

Think I'm happy with my final review 🙌

Shall we update the description to add @include govuk-text-colour; has been added where missed too?

Or split that out into another issue? I've not checked if we've caught them all

@owenatgov
Copy link
Contributor Author

@colinrotherham I've added it to the bottom of the description of this PR. I'm fairly confident we're using govuk-text-colour across our library so I don't think it needs a new issue right now.

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.

Looking rather good 🙌

Can we have an updated file size saving figure? Unless it's bigger 😮

@owenatgov
Copy link
Contributor Author

@colinrotherham Interestingly, the minified CSS hasn't changed but the map has increased by one kb 😮 I wonder how much of this is impacted by parallel work and rebasing. Not an analysis worth stressing over right now. Merging!

@owenatgov owenatgov merged commit 9f7c224 into main Oct 2, 2023
44 checks passed
@owenatgov owenatgov deleted the remove-improper-use-of-govuk-font branch October 2, 2023 09:56
@colinrotherham
Copy link
Contributor

Thanks @owenatgov

We've added more Sass than we've removed I guess? All those source changes needs mapping

They're are only downloaded when developer tools are open so it's fine by me

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.

Check improper use of govuk-font within GOV.UK Frontend
4 participants