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.

Sign up for GitHub

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

Refactor back link and breadcrumb chevrons to use ems #2998

Merged
merged 7 commits into from
Nov 29, 2022
Merged

Refactor back link and breadcrumb chevrons to use ems #2998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2079481
Change back link arrow measurements to use ems
querkmachine Nov 10, 2022
e5a7e30
Add changelog entry
querkmachine Nov 10, 2022
767f13e
Switch to using govuk-em function
querkmachine Nov 10, 2022
253d487
Mirror changes onto breadcrumbs component chevrons
querkmachine Nov 11, 2022
ab937f4
Add minimum border thickness to chevrons
querkmachine Nov 14, 2022
b0d4ee7
Use max() to enforce a minimum chevron size
querkmachine Nov 28, 2022
33d69cb
Update changelog
querkmachine Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Fixes

- [#2998: Refactor back link and breadcrumb chevrons to use ems](https://github.com/alphagov/govuk-frontend/pull/2998)

## 4.4.0 (Feature release)

### New features
Expand Down
38 changes: 19 additions & 19 deletions src/govuk/components/back-link/_index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
@include govuk-exports("govuk/component/back-link") {
// Component font-size on the Frontend (used for calculations)
$font-size: 16;

// Size of chevron (excluding border)
$chevron-size: 7px;
$chevron-size: govuk-em(7px, $font-size);

// Size of chevron border
$chevron-border-width: 1px;
$chevron-border-min-width: 1px;
$chevron-border-width: govuk-em($chevron-border-min-width, $font-size);

// Colour of chevron
$chevron-border-colour: $govuk-secondary-text-colour;

.govuk-back-link {
@include govuk-typography-responsive($size: 16);
@include govuk-typography-responsive($size: $font-size);
@include govuk-link-common;
@include govuk-link-style-text;

Expand All @@ -21,7 +24,7 @@
margin-bottom: govuk-spacing(3);

// Allow space for the arrow
padding-left: 14px;
padding-left: govuk-em(14px, $font-size);
}

// Prepend left pointing chevron
Expand All @@ -35,14 +38,15 @@
@if $govuk-use-legacy-font {
// Begin adjustments for font baseline offset
// These should be removed when legacy font support is dropped
top: -1px;
bottom: 1px;
$offset: govuk-em(1px, $font-size);
top: $offset * -1;
bottom: $offset;
} @else {
top: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

@querkmachine You've got me looking at this more now 🙈

Do you think we should let browsers calculate the vertical position instead of assuming top: 0?

For example, applying the old 50% vertical offset trick puts the chevron about 1px lower on mobile. I've added an example from macOS Safari 16.1 with 2x window.devicePixelRatio—looks better aligned?

1st animation frame — original top: 0
2nd animation frame — 50% offset (suggestion)

Back link animation showing 1px vertical shift

Suggested change
top: 0;
top: 50%;
margin-top: $chevron-size / -2

Copy link
Member Author

Choose a reason for hiding this comment

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

This might be a question for the designers, but I think the intent is that the chevron is aligned with the text baseline, rather than being exactly vertically centred?

Copy link
Contributor

@colinrotherham colinrotherham Nov 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah best check, Percy shows it well?

As in, small screens are now (correctly) showing a smaller chevron but it's too high

See how it shifts up ꜛ (not down ꜜ to the baseline) at the new smaller chevron size:

Back link animation showing 1px vertical shift (Percy)

bottom: 0;
}

left: 3px;
left: govuk-em(3px, $font-size);

width: $chevron-size;
height: $chevron-size;
Expand All @@ -52,9 +56,16 @@
transform: rotate(225deg);

border: solid;
border-width: $chevron-border-width $chevron-border-width 0 0;
border-width: $chevron-border-min-width $chevron-border-min-width 0 0;
border-color: $chevron-border-colour;

@supports (border-width: unquote("max(0px)")) {
border-width: unquote("max(#{$chevron-border-min-width}, #{$chevron-border-width}) max(#{$chevron-border-min-width}, #{$chevron-border-width})") 0 0;

// Ensure that the chevron never gets smaller than 16px
font-size: unquote("max(#{$font-size * 1px}, 1em)");
}

// Fall back to a less than sign for IE8
@include govuk-if-ie8 {
content: "\003c"; // Less than sign (<)
Expand All @@ -81,15 +92,4 @@
bottom: -14px;
left: 0;
}

@if $govuk-use-legacy-font {
// Begin adjustments for font baseline offset
// These should be removed when legacy font support is dropped
.govuk-back-link:before {
$offset: 1px;

top: $offset * -1;
bottom: $offset;
}
}
}
31 changes: 21 additions & 10 deletions src/govuk/components/breadcrumbs/_index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
@include govuk-exports("govuk/component/breadcrumbs") {
// Component font-size on the Frontend (used for calculations)
$font-size: 16;

// Size of chevron (excluding border)
$chevron-size: 7px;
$chevron-size: govuk-em(7px, $font-size);

// Size of chevron border
$chevron-border-width: 1px;
$chevron-border-min-width: 1px;
$chevron-border-width: govuk-em($chevron-border-min-width, $font-size);

// Colour of chevron
$chevron-border-colour: $govuk-secondary-text-colour;
Expand All @@ -13,10 +16,10 @@
// of length 8 (7px + 1px border):
//
// √(8² + 8²) * 0.5 ≅ 5.655
$chevron-altitude-calculated: 5.655px;
$chevron-altitude-calculated: govuk-em(5.655px, $font-size);
querkmachine marked this conversation as resolved.
Show resolved Hide resolved

.govuk-breadcrumbs {
@include govuk-font($size: 16);
@include govuk-font($size: $font-size);
@include govuk-text-colour;

margin-top: govuk-spacing(3);
Expand All @@ -40,8 +43,8 @@

// Add both margin and padding such that the chevron appears centrally
// between each breadcrumb item
margin-left: govuk-spacing(2);
padding-left: govuk-spacing(2) + $chevron-altitude-calculated;
margin-left: govuk-em(govuk-spacing(2), $font-size);
padding-left: govuk-em(govuk-spacing(2), $font-size) + $chevron-altitude-calculated;

float: left;

Expand All @@ -55,8 +58,9 @@
@if $govuk-use-legacy-font {
// Begin adjustments for font baseline offset
// These should be removed when legacy font support is dropped
top: -1px;
bottom: 1px;
$offset: govuk-em(1px, $font-size);
top: $offset * -1;
bottom: $offset;
} @else {
top: 0;
bottom: 0;
Expand All @@ -74,9 +78,16 @@
transform: rotate(45deg);

border: solid;
border-width: $chevron-border-width $chevron-border-width 0 0;
border-width: $chevron-border-min-width $chevron-border-min-width 0 0;
border-color: $chevron-border-colour;

@supports (border-width: unquote("max(0px)")) {
border-width: unquote("max(#{$chevron-border-min-width}, #{$chevron-border-width}) max(#{$chevron-border-min-width}, #{$chevron-border-width})") 0 0;

// Ensure that the chevron never gets smaller than 16px
font-size: unquote("max(#{$font-size * 1px}, 1em)");
}

// Fall back to a greater than sign for IE8
@include govuk-if-ie8 {
content: "\003e"; // Greater than sign (>)
Expand Down Expand Up @@ -118,7 +129,7 @@
}

&:before {
top: 6px;
top: govuk-em(6px, $font-size);
margin: 0;
}
}
Expand Down