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.

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

Merged
merged 4 commits into from
Jun 14, 2024
Merged

Conversation

SaptakS
Copy link
Contributor

@SaptakS SaptakS commented Jun 11, 2024

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:

  • Make all anchor tags have a solid 2px outline of the currentColor (Sadly the currentColor itself doesn't meet WCAG color contrast requirement, so the focus indicator doesn't either, but hopefully that's a separate discussion)
  • Apply the same above design to all buttons as well
  • Adds an offsetted focus outline for the CTAs
  • Moves padding in navigation elements from the <a> element to the <li> element.

These address the below concerns by @thibaudcolas in #1381

Focus style almost invisible on "Get started with Django" CTA

Custom "dotted outline" focus styles: not visible enough and broken in main navigation

Attaching some screenshots of the changes:
image
image

Moves the padding from the a element to the li element such that the
focus indicators get better rendered.
@sabderemane sabderemane requested review from thibaudcolas, sabderemane and a team June 12, 2024 15:05
Copy link
Member

@bmispelon bmispelon left a 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! ✨

Comment on lines 622 to 623
@include respond-min(768px) {
padding: 20px 10px;
}
Copy link
Member

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?

@@ -1229,11 +1237,15 @@ a.cta {
font-style: normal;
}

&:hover,
&:focus {
&:hover{
Copy link
Member

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:

Suggested change
&:hover{
&:hover {

@SaptakS
Copy link
Contributor Author

SaptakS commented Jun 13, 2024

Thanks @bmispelon for pointing those out. Have updated them.

@bmispelon
Copy link
Member

Thanks for the quick update. This looks good to me now, unless anyone objects I'll merge and deploy this over the coming weekend.

@bmispelon bmispelon merged commit f11c87e into django:main Jun 14, 2024
2 checks passed
@bmispelon
Copy link
Member

And it's live, well done everybody 👏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants