Conversation
🦋 Changeset detectedLatest commit: 534c8f6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
koddsson
left a comment
koddsson
left a comment
There was a problem hiding this comment.
This looks great to me ✨
| &:focus { | ||
| z-index: 1; | ||
| outline: none; | ||
| box-shadow: var(--color-state-focus-shadow); | ||
| } |
There was a problem hiding this comment.
This might be a bigger accessibility question but should/could we make this a default for all focusable elements?
There was a problem hiding this comment.
Hmm.. yeah. Currently we have a mix between native focus (e.g. links) and custom focus (input, button). It would be nice to unify it:
*:focus {
outline: none;
box-shadow: var(--color-state-focus-shadow);
}
But we'd have to tweak existing components like the underline nav:
Another option might be to only change the outline color:
*:focus {
outline-color: var(--color-border-info);
}
There was a problem hiding this comment.
Added an issue here: https://github.com/github/primer/issues/160
2 participants
This makes the
:focusstate more visible for.SideNavand.menu. Currently only the background changes, but it's too subtle.It doesn't look that great, but maybe still better than only have a subtle change in background. Also, the these two components will probably be replaced sooner or later.