Only underline logo on hover (with custom colours) #926
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-926
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-926
NickColley
reviewed
Jul 23, 2018
src/components/header/_header.scss
Outdated
| border-bottom-color: currentColor; | ||
| margin-bottom: -1px; // Negate transparent bottom border | ||
| border-bottom: 1px solid currentColor; | ||
| @include govuk-if-ie8 { |
There was a problem hiding this comment.
could you include a comment that explains why the ie8 code is needed? the info in the PR is very good
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-926
NickColley
reviewed
Jul 23, 2018
src/components/header/_header.scss
Outdated
| @@ -108,6 +108,8 @@ | |||
| &:active { | |||
| margin-bottom: -1px; // Negate transparent bottom border | |||
| border-bottom: 1px solid currentColor; | |||
There was a problem hiding this comment.
Out of curiosity, I think that border's default colour is currentColor which could mean changing this statement to border-bottom: 1px solid may remove the need for the conditional IE8 logic?
There was a problem hiding this comment.
Great suggestion – that seems to work well.
NickColley
approved these changes
Jul 23, 2018
Because all borders will become text colour when a user overrides colour in their browser, the transparent border will become always visible. By only applying the border on hover we avoid this which means we get the correct behaviour even when colours are customised. The layout is not affected because we offset the additional border with a negative margin. Removing any border-color attribute means that we'll use the currentColor for the border - making this explicit results in IE8 ignoring the rule because it doesn't understand the currentColor variable.
36degrees
force-pushed
the
header-logo-underline-on-hover
branch
from
3a500b7
to
6ce51bf
Compare
|
@NickColley would you mind taking another look? |
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Because all borders will become text colour when a user overrides colour in their browser, the transparent border will become always visible.
By only applying the border on hover we avoid this which means we get the correct behaviour even when colours are customised. The layout is not affected because we offset the additional border with a negative margin.
We do however have to add an additional rule for IE8 which does not support customColor and so ignores the rule entirely - which meant it had no border before. This would be fine, except the negative margin does kicks in on hover, which means without a border the header would shrink by 1px on hover in IE8.
Before
After