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

Removing font style adjustments #1441

Merged
merged 6 commits into from
Jun 11, 2019
Merged

Removing font style adjustments #1441

merged 6 commits into from
Jun 11, 2019

Conversation

aliuk2012
Copy link
Contributor

@aliuk2012 aliuk2012 commented Jun 10, 2019

Based off @dashouse initial spike #1356 and linked to #1434.

This PR moves the adjustments behind a sass variable called $govuk-use-legacy-font. This should mean that services not running in compatibility mode should have a slightly smaller compiled CSS without the adjustments.

Components checked

  • Accordion
  • Back-link
  • Breadcrumbs
  • Button
  • Character count
  • Checkboxes
  • Date-input
  • Details
  • error-messages
  • fieldset
  • file-upload
  • footer
  • header
  • hint
  • input
  • inset-text
  • Label
  • panel
  • Phase-banner
  • Radios
  • Select
  • Skip-link
  • Summary-list
  • Table
  • tabs
  • Tag
  • Textarea
  • Warning-text

Components affected and updated

  • Back-link
  • Breadcrumbs
  • Button (mostly affected start button)
  • header
  • tags

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1441 June 10, 2019 07:44 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1441 June 10, 2019 12:00 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1441 June 10, 2019 12:24 Inactive
@aliuk2012 aliuk2012 marked this pull request as ready for review June 10, 2019 12:35
@aliuk2012 aliuk2012 added this to the v3.0.0 milestone Jun 10, 2019
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

The header in legacy mode seems quite a lot bigger, is this intentional?

Apart from that I've looked at the before and after with legacy mode and it all looks good to me.

Would be good to get @dashouse 's sign off.

left: 0;

margin: auto;
}
}

// Begin adjustments for font baseline offset
// These should be removed when the font is updated with the correct baseline
@if $govuk-use-legacy-font {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like using this instead of compatibility mode, feels more explicit 👍

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1441 June 10, 2019 14:14 Inactive
@dashouse
Copy link

Looks good to me 👏

padding-left: 8px;
} @else {
@include govuk-font($size: 16, $weight: bold, $line-height: 1);
padding-top: 5px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking this could be short hand as a one liner.

Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Excellent work both 👏

Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Needs a CHANGELOG entry

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1441 June 11, 2019 11:23 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1441 June 11, 2019 11:23 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants