-
Notifications
You must be signed in to change notification settings - Fork 320
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
Focus iteration #1245
Focus iteration #1245
Conversation
c024d24
to
d4e8f7b
Compare
6dd0ea4
to
1f3390b
Compare
Create page for examples Add examples Update examples Update content Temporarily disable error summary js Update button information Fix typos Add information about border on tabs Update based on clarification with a11y team Add link class to links in buton section Update focusable fill style to include border Focus and component changes
6bb92e8
to
393f7c4
Compare
$govuk-footer-link: $govuk-footer-text; | ||
$govuk-footer-link-hover: #171819; | ||
$govuk-footer-link-hover: $govuk-footer-text; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the above has already been done.
display: inline; | ||
margin: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From talking to @dashouse, this is removing the user agent default styling.
@@ -22,7 +22,7 @@ | |||
margin-bottom: govuk-spacing(1); | |||
|
|||
// Allow for absolutely positioned marker and align with disclosed text | |||
padding-left: govuk-spacing(4) + $govuk-border-width; | |||
padding-left: govuk-spacing(3) + $govuk-border-width; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -125,6 +125,7 @@ | |||
// Since box-shadows are removed when users customise their colours, we set | |||
// a transparent outline that is shown instead. | |||
// https://accessibility.blog.gov.uk/2017/03/27/how-users-change-colours-on-websites/ | |||
border: 4px solid govuk-colour("black"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this can just be border-width
@@ -95,9 +111,9 @@ | |||
position: absolute; | |||
|
|||
top: -$govuk-border-width-form-element; | |||
right: -$govuk-border-width-form-element; | |||
right: -5px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if these changes are required or not.
@@ -97,12 +108,17 @@ | |||
padding-left: govuk-spacing(4) - 1px; | |||
|
|||
border: 1px solid $govuk-border-colour; | |||
border-top: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This affects tabs with overridden colours - the top border disappears. May want to try setting it to transparent instead?
@@ -125,6 +125,7 @@ | |||
// Since box-shadows are removed when users customise their colours, we set | |||
// a transparent outline that is shown instead. | |||
// https://accessibility.blog.gov.uk/2017/03/27/how-users-change-colours-on-websites/ | |||
border: 4px solid govuk-colour("black"); | |||
outline: $govuk-focus-width solid transparent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be required any more, with the border-width change.
@@ -126,6 +126,7 @@ | |||
// Since box-shadows are removed when users customise their colours we set a | |||
// transparent outline that is shown instead. | |||
// https://accessibility.blog.gov.uk/2017/03/27/how-users-change-colours-on-websites/ | |||
border: 4px solid govuk-colour("black"); | |||
outline: $govuk-focus-width solid transparent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not be required any more either (as with checkboxes)
@@ -31,24 +31,29 @@ | |||
@mixin govuk-link-style-default { | |||
&:link { | |||
color: $govuk-link-colour; | |||
text-decoration-color: govuk-colour("light-blue"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're now not making this change (at least for now)
@@ -25,6 +25,11 @@ | |||
// Underline is provided by a bottom border | |||
text-decoration: none; | |||
|
|||
|
|||
&:focus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may be able to close #1236 with the focus state change here.
|
||
&:link, | ||
&:visited { | ||
color: $govuk-footer-link; | ||
text-decoration: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dashouse are we sure about removing the underline in the footer, it makes all the links look like body content, there's no way of knowing something is clickable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm im presuming this change it to make the footer consistent with the header...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to go with this for now since it fits our navigation patterns.
I believe that this could fail WCAG though:
@@ -30,5 +29,28 @@ | |||
outline: $govuk-focus-width solid $govuk-focus-colour; | |||
outline-offset: 0; | |||
background-color: $govuk-focus-colour; | |||
text-decoration: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dashouse can you confirm that you want to remove text-decoration here? It's think it's only used in the skip-link component after these changes, which I'm not sure is a good idea since the underline helps give it the affordance that it can be clicked?
We want to review these mixins once we've done all the work but just wondering here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only affect links that are focussed, I think? I believe the intention is to avoid a 'double underline' from the text decoration and the black box shadow together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mixin is no longer used with links, I think you're replying thinking of the mixin below this one potentially?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke to Dave and given that this is only used by tabbing to the component we've decided to leave the underline in for now.
WIP