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

Adjust tag component padding to compensate for font spacing #955

Merged
merged 2 commits into from
Aug 14, 2018

Conversation

NickColley
Copy link
Contributor

@NickColley NickColley commented Aug 10, 2018

This is in response to an issue raised on the backlog.

Until we update GOV.UK Frontend with the new font, NTA
sits higher than common fonts, so we need to add adjustments
to compensate.

Links for review:

Screenshots

Before changes

Tag using default font with no changes

screen shot 2018-08-10 at 16 49 54


After changes

Tag using default font with changes

screen shot 2018-08-10 at 16 50 59

https://trello.com/c/GV0PxHFL/1319-fix-vertical-text-alignment-in-tag-component

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-955 August 10, 2018 15:57 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-955 August 10, 2018 16:09 Inactive
@NickColley
Copy link
Contributor Author

Note that this adjusts the text it sits beside slightly:

screen shot 2018-08-10 at 17 12 00

@NickColley NickColley changed the title Adjust Tag padding to compensate for font spacing Adjust tag component padding to compensate for font spacing Aug 10, 2018
@NickColley NickColley force-pushed the add-custom-spacing-to-tag-for-font branch from 5a7dbfb to edd5d39 Compare August 10, 2018 16:17
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-955 August 10, 2018 16:17 Inactive
@NickColley NickColley force-pushed the add-custom-spacing-to-tag-for-font branch from edd5d39 to 7d3b816 Compare August 10, 2018 16:23
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-955 August 10, 2018 16:23 Inactive
@NickColley NickColley force-pushed the add-custom-spacing-to-tag-for-font branch from 7d3b816 to e07a915 Compare August 10, 2018 16:24
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-955 August 10, 2018 16:24 Inactive
@36degrees 36degrees added this to the [NEXT] milestone Aug 13, 2018
// Since New Transport sits slightly higher than other common fonts.
// We use intentionally uneven padding to make it balanced, this can be
// removed using the version of the font that has a more common vertical spacing.
@if $govuk-font-family == $govuk-font-family-nta {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this isn't true, then the alpha tag ends up with 4px on top and bottom, which makes it 28px – considerably larger than the 'with nta' variant and no longer following our /5 rhythm. I think if we're going to try and make this 'conditional' we should make sure that the component still follows our rules in both cases.

In other places we're just commenting and applying adjustments by taking from one side and adding to the other, e.g. like this: https://github.com/alphagov/govuk-frontend/blob/master/src/components/button/_button.scss#L164-L178

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought process:

  1. As far as I know the 5 pixels is a scale based on how NTA works (might be wrong)
  2. The component being a little bit bigger is much better than it looking completely broken as seen in the screenshots.

I'll make this less clever and we'll figure this out when we do proper theming support.

Choose a reason for hiding this comment

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

The 5px thing is not related to the font, it's a decision based on the font-size / line-height combination. Each font size has a line-height rounded to the nearest 5px increment. This is why the spacing scale is increments of 5px and 10px and the inputs, buttons etc have been standardised at 40px (45px with the start button)

I say this, but this has mainly been considered for the main content of the page, there were way too many variables to make it work properly for the header and beforeContent area...haven't checked it in a while.

@NickColley NickColley force-pushed the add-custom-spacing-to-tag-for-font branch from e07a915 to 9f299fd Compare August 13, 2018 15:51
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-955 August 13, 2018 15:52 Inactive
Until we update GOV.UK Frontend with the new font, NTA
sits higher than common fonts, so we need to add adjustments
to compensate.
@NickColley NickColley merged commit 28086d4 into master Aug 14, 2018
@36degrees 36degrees deleted the add-custom-spacing-to-tag-for-font branch August 14, 2018 13:05
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