Disable ink skipping for underlines in hover state #2251
Conversation
christopherthomasdesign
changed the title
CharlotteDowns
changed the title
CharlotteDowns
changed the title
|
Is it worth also including |
We use autoprefixer which will take care of that for us, based on our browser support requirements. |
The thick underlines we use in the new hover states are rendered differently by different browsers. In Safari and Firefox, links can look messy if the link text contains words with lots of descenders. Disable ink skipping on hover, which means that links look closer to the way they do in Safari currently. Use both `text-decoration-skip-ink: none;` and `text-decoration-skip: none;` – Safari supports `text-decoration-skip` whilst other browsers support `text-decoration-skip-ink`. Fun!
36degrees
force-pushed
the
link-underline-adjustment
branch
from
386e47d
to
e873ed5
Compare
36degrees
changed the title
| @@ -121,7 +121,7 @@ describe('@mixin govuk-link-hover-decoration', () => { | |||
| }) | |||
|
|
|||
| describe('when $govuk-new-link-styles are enabled', () => { | |||
| it('sets text-decoration-thickness on hover', async () => { | |||
| it('sets a hover state', async () => { | |||
There was a problem hiding this comment.
Should we leave this test as it is, and add a new test for ink skipping that tests for the presence of the styles?
There was a problem hiding this comment.
The test has to change because it no longer passes:
● @mixin govuk-link-hover-decoration › when $govuk-new-link-styles are enabled › sets text-decoration-thickness on hover
expect(received).toContain(expected) // indexOf
Expected substring: ".foo:hover { text-decoration-thickness: 10px; }"
Received string: ".foo:hover { text-decoration-thickness: 10px; text-decoration-skip-ink: none; text-decoration-skip: none; }
We could add tests for each of the properties, but I don't think we want to test the CSS itself, just the logic that surrounds it. In this case, we want to know how the mixin behaves differently depending on the various link settings:
- by default there's no hover state
- when
$govuk-new-link-stylesare enabled, there is a hover state (but we don't care exactly what that hover state looks like – that's what manual testing is for) - except when
$govuk-link-hover-underline-thicknessis falsey, in which case the hover state is removed
I think the revised tests cover this. What do you think?
There was a problem hiding this comment.
Thanks @36degrees that covers it 👍
Solves #2250
Uses both
text-decoration-skip-ink: none;andtext-decoration-skip: none;to stop the thick hover underlines from skipping the descenders. We have to use both because Safari supportstext-decoration-skipand the other browsers supporttext-decoration-skip-ink. Fun!Before (Safari)
After (Safari)
Before (Firefox)
After (Firefox)
Before (Chrome)
After (Chrome)