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

Use relative line-height #837

Merged
merged 2 commits into from
Jun 28, 2018
Merged

Use relative line-height #837

merged 2 commits into from
Jun 28, 2018

Conversation

dashouse
Copy link

@dashouse dashouse commented Jun 27, 2018

  • Converts line heights to relative unit-less value

This needs to be tested on Samsung Internet browser to see if it fixes issue brought up in Accessibility audit.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-837 June 27, 2018 08:38 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-837 June 27, 2018 11:57 Inactive
@dashouse dashouse changed the title RFC - Typographic units Use relative line-height Jun 27, 2018
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-837 June 27, 2018 11:58 Inactive
Copy link

@kr8n3r kr8n3r left a comment

Choose a reason for hiding this comment

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

good stuff

Copy link

@kr8n3r kr8n3r left a comment

Choose a reason for hiding this comment

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

can you add a changelog entry please

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.

Can we please test this:

  • across browsers/devices
  • where possible where we have the font text size changed (e.g. samsung browser)

before merging since it's a pretty wide reaching change.

@hannalaakso
Copy link
Member

hannalaakso commented Jun 28, 2018

I've tested this on the following and it looks good:

  • Chrome (latest) on Mac
  • Chrome (Samsung Android) with text resized to 200% in Chrome settings
  • Samsung Internet (Samsung Android) with text resized to "Huge" in phone settings
  • Firefox (latest) on Mac
  • Safari 11.1 on Mac
  • iOS 11 Safari with text resized to max size in phone settings
  • IE8-11 on Windows

To test, I made the examples in examples/typography longer in order to check text wrapping.

Just need a changelog entry for the PR.

@NickColley
Copy link
Contributor

Firefox before and after:

screen shot 2018-06-28 at 11 11 45

screen shot 2018-06-28 at 11 11 40

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-837 June 28, 2018 12:23 Inactive
CHANGELOG.md Outdated
@@ -20,6 +20,10 @@

([PR #N](https://github.com/alphagov/govuk-frontend/pull/N))

- Use relative line-height
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're not changing the public api, and we're fixing a behaviour I think this might be a 'fix' instead of a 'feature'

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-837 June 28, 2018 12:26 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-837 June 28, 2018 12:36 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-837 June 28, 2018 12:37 Inactive
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.

This is a great improvement 👏

@@ -26,11 +26,11 @@ $govuk-typography-scale: (
80: (
null: (
font-size: 53px,
line-height: 55px
line-height: 1.0377358491 // 55px
Copy link
Member

Choose a reason for hiding this comment

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

I do wonder if these would have been clearer if written as:

line-height: 55px / 53px;

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could have left these as pixel values in the font-map and calculated them in the typography-responsive mixin.

Copy link
Author

Choose a reason for hiding this comment

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

I tried various ways of doing this calculation but kept running into issues...Should probably refactor so it's more "automatic" / less prone to typos. If we haven't released can we carry on?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't expect this to change often, might be worth doing less here?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, happy with this. I've been playing locally with this too – looks like our font-size and line-height are ending up as strings somehow, which means using them in calculations doesn't work. It'd be great to be able to define everything in pixels and everything 'just work', but happy with this for now 👍

Copy link
Author

Choose a reason for hiding this comment

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

I kind of got something interesting when spitting it out as a string if we change the output to font: 16 / 20 and not define line height independently... but this didn't play very nicely with the other PR that needed two font-size declarations.

@kr8n3r kr8n3r added this to the v1.1.0 milestone Jul 4, 2018
@NickColley NickColley mentioned this pull request Jul 13, 2018
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.

None yet

6 participants