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 legacy and tabular fonts support #3574

Merged
merged 3 commits into from
May 4, 2023

Conversation

querkmachine
Copy link
Member

Removes $govuk-use-legacy-font and $govuk-font-family-tabular settings.

Resolves #2791.

Changes

  • Removes component code that's conditional on $govuk-use-legacy-font being true.
  • Removes code that's conditional on $govuk-font-family-tabular being true from govuk-font mixin.
  • Removes tests relating to $govuk-font-family-tabular.
  • Removes $govuk-use-legacy-font, $govuk-font-family-tabular, $govuk-font-family-nta and $govuk-font-family-nta-tabular settings.
  • Updates $govuk-font-family setting to always use GDS Transport by default.
  • Updates tests that used $govuk-font-family-tabular as dummy data (usually as a shorthand for a non-GDS Transport font) to use Helvetica instead.

@querkmachine querkmachine requested a review from a team May 4, 2023 11:06
@querkmachine querkmachine self-assigned this May 4, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3574 May 4, 2023 11:06 Inactive
@querkmachine querkmachine changed the title Remove legacy and legacy tabular fonts support Remove legacy and tabular fonts support May 4, 2023
@querkmachine querkmachine linked an issue May 4, 2023 that may be closed by this pull request
8 tasks
@@ -54,7 +54,7 @@ describe('@mixin govuk-typography-common', () => {
@include govuk-typography-common;
}
:root {
@include govuk-typography-common($font-family: $govuk-font-family-tabular);
@include govuk-typography-common($font-family: Helvetica);
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels odd to have Helvetica here without a matching check for the output?

I appreciate we didn't before this edit, but we did (further down) in the tabular test

await expect(results).resolves.toMatchObject({
  css: expect.stringContaining('font-family: Helvetica')
})

await expect(results).resolves.toMatchObject({
  css: expect.stringContaining('font-family: "GDS Transport"')
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker

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 brilliant

(I'm just going to rebase this btw, will let you know when it's done)

@colinrotherham colinrotherham force-pushed the remove-compat-mode-legacy-fonts branch from 429e562 to dc875b7 Compare May 4, 2023 11:37
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3574 May 4, 2023 11:37 Inactive
Base automatically changed from remove-ie8-css to main May 4, 2023 14:04
@querkmachine querkmachine merged commit dc5d5fa into main May 4, 2023
@querkmachine querkmachine deleted the remove-compat-mode-legacy-fonts branch May 4, 2023 15:42
romaricpascal pushed a commit that referenced this pull request May 18, 2023
@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.

Remove legacy font setting and tabular font separation setting from codebase
3 participants