Fixed #2557 -- Improved color contrast for the Django logo focus indicator#2558
Conversation
erosselli
left a comment
erosselli
left a comment
There was a problem hiding this comment.
Hello! Thanks for contributing! I don't think this quite fixes the issue, the updates are for the logo on the footer (which is already OK since it was fixed in PR #2540) but the issue is asking for changes on the logo for the main navbar
| outline: 2px solid var(--menu); | ||
| outline: 2px solid var(--body-fg); | ||
| } |
There was a problem hiding this comment.
this is the logo within the footer, which already has the white focus indicator and it's visible (see how it's nested under the .footer class) . The ticket is talking about the logo on the main navabar at the top left of the page
|
Hello Eli,I have updated the fix, logo focus outline is now white in both light and dark mode. Before 
After 
|
There was a problem hiding this comment.
Thanks for contributing! I left some comments.
Let's also make sure the PR title follows our commit guidelines , and contains info about the problem/fix (e.g "Fixed #2557 -- Improved color contrast for the Django logo focus indicator.") rather than its specific implementation ("Use --body-fg variable ...")
| } | ||
| } | ||
|
|
||
| /* Fix logo focus outline in dark mode - header is always dark green */ |
There was a problem hiding this comment.
I don't think this comment is really helping anything, we should probably remove it
| html[data-theme="dark"] header .logo:focus { | ||
| outline: 2px solid #ffffff !important; | ||
| outline-offset: 2px; | ||
| } | ||
|
|
||
| @media (prefers-color-scheme: dark) { | ||
| html:not([data-theme="light"]) header .logo:focus { | ||
| outline: 2px solid #ffffff !important; | ||
| outline-offset: 2px; | ||
| } | ||
| } |
There was a problem hiding this comment.
more generally, I don't think these changes are needed. If something needs a lot of !important statements that's usually a sign that we're not coding our CSS in the best way
| &:focus { | ||
| outline: 2px solid var(--white-color) !important; | ||
| outline-offset: 2px; | ||
| } |
There was a problem hiding this comment.
I think you should be able to reduce this down to , that's what we have in the footer and it's working fine.
&:focus {
outline: 2px solid var(--menu);
}
Can you explain why you added the outline-offset here? I don't think we have that offset in our other focus indicators
| &:focus { | ||
| outline: 2px solid var(--menu); | ||
| outline: 2px solid var(--body-fg); |
There was a problem hiding this comment.
this is an unrelated change and not needed, please undo it -- the footer focus indicator already works as expected
bryan-oginga
changed the title
bryan-oginga
force-pushed
the
fix/dark-mode-logo-focus-indicator
branch
from
5fbb9b2 to
6470f4f
Compare
|
Thanks Eli for your review! I've addressed all the comments and reverted the unrelated footer change |
bryan-oginga
force-pushed
the
fix/dark-mode-logo-focus-indicator
branch
2 times, most recently
from
6c38c64 to
16649a8
Compare
for more information, see https://pre-commit.ci
github-project-automation
Bot
moved this from New
to Done
in django accessibility improvements
3 participants
In dark mode, the Django logo focus indicator used var(--menu)
which renders as black against the dark green footer background,
making it nearly invisible and failing accessibility contrast requirements.
Changed to var(--body-fg) which renders as #C1CAD2 (light gray)
in dark mode, matching other focus indicators on the page and
providing sufficient contrast.