-
-
Notifications
You must be signed in to change notification settings - Fork 943
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
Fixes focus indicators #1545
Fixes focus indicators #1545
Conversation
Moves the padding from the a element to the li element such that the focus indicators get better rendered.
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 seems like a good change. I've tested it locally and it works.
I have just two minor comments which don't affect the feature itself.
Thanks for working on this! ✨
djangoproject/scss/_style.scss
Outdated
@include respond-min(768px) { | ||
padding: 20px 10px; | ||
} |
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.
The whole @include
is now empty, so it can be deleted, right?
djangoproject/scss/_style.scss
Outdated
@@ -1229,11 +1237,15 @@ a.cta { | |||
font-style: normal; | |||
} | |||
|
|||
&:hover, | |||
&:focus { | |||
&:hover{ |
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.
Missing a space before the bracket:
&:hover{ | |
&:hover { |
Thanks @bmispelon for pointing those out. Have updated them. |
Thanks for the quick update. This looks good to me now, unless anyone objects I'll merge and deploy this over the coming weekend. |
And it's live, well done everybody 👏🏻 |
Refs #1381
Improves the focus indicators. Based on WCAG 2.2 standards, it is a better idea to have solid outlines instead of a dotted outline for focus indicators. Also, color change for focus indication isn't great because it doesn't often work for color blinded people.
So things done in this PR:
<a>
element to the<li>
element.These address the below concerns by @thibaudcolas in #1381
Attaching some screenshots of the changes: