-
Notifications
You must be signed in to change notification settings - Fork 317
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
Consistent focus ring (first pass) #1549
Changes from 9 commits
4062f89
1d5af58
5856c9e
99dcef3
850fc9c
357b34c
7470f40
37640dd
e2d1810
94e6804
f3f53c0
029d178
b4c08a9
e67849e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,8 +58,6 @@ | |
&:focus, | ||
&:focus-visible { | ||
border: none; | ||
box-shadow: none; | ||
outline: 3px solid var(--pst-color-accent); | ||
Comment on lines
-61
to
-62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new |
||
background-color: var(--pst-color-background); | ||
color: var(--pst-color-text-muted); | ||
} | ||
|
@@ -75,11 +73,16 @@ | |
align-items: center; | ||
align-content: center; | ||
color: var(--pst-color-text-muted); | ||
// Needed to match other icons hover | ||
padding: 0 0 0.25rem 0; | ||
border-radius: 0; | ||
border: none; // Override Bootstrap button border | ||
font-size: 1rem; // Override Bootstrap button font size | ||
Comment on lines
+77
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For both the search button and the theme switcher button, I decided to go ahead and make them consistent with the other icons at the end of the nav bar. |
||
|
||
// Override Bootstrap button padding-x. Whitespace in nav bar is controlled | ||
// via column gap rule on the container. | ||
padding-left: 0; | ||
padding-right: 0; | ||
|
||
@include icon-navbar-hover; | ||
@include focus-indicator; | ||
|
||
i { | ||
font-size: 1.3rem; | ||
|
@@ -136,19 +139,21 @@ | |
* Lives at components/search-button-field.html | ||
*/ | ||
.search-button-field { | ||
$search-button-border-radius: 1.5em; | ||
display: inline-flex; | ||
align-items: center; | ||
border: var(--pst-color-border) solid 1px; | ||
border-radius: 1.5em; | ||
border-radius: $search-button-border-radius; | ||
color: var(--pst-color-text-muted); | ||
padding: 0.5em; | ||
background-color: var(--pst-color-surface); | ||
|
||
&:hover { | ||
border: 2px solid var(--pst-color-link-hover); | ||
} | ||
|
||
&:focus-visible { | ||
border: 2px solid var(--pst-color-accent); | ||
border-radius: $search-button-border-radius; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed to override a rule I added in |
||
} | ||
|
||
// The keyboard shotcut text | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,18 +3,23 @@ | |
*/ | ||
|
||
.theme-switch-button { | ||
// overide bootstrap settings | ||
margin: 0 -0.5rem; | ||
padding: 0; // We pad the `span` not the container | ||
color: var(--pst-color-text-muted); | ||
border-radius: 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now gets a rounded focus ring so I decided to remove this border-radius rule. |
||
@include focus-indicator; | ||
border: none; // Override Bootstrap button border | ||
font-size: 1rem; // Override Bootstrap's button font size | ||
|
||
// Override Bootstrap button padding-x. Whitespace in nav bar is controlled | ||
// via column gap rule on the container. | ||
padding-left: 0; | ||
padding-right: 0; | ||
|
||
&:hover { | ||
@include icon-navbar-hover; | ||
} | ||
|
||
span { | ||
display: none; | ||
padding: 0.5em; | ||
|
||
@include icon-navbar-hover; | ||
&:active { | ||
text-decoration: none; | ||
color: var(--pst-color-link-hover); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,4 +48,8 @@ nav.page-toc { | |
} | ||
} | ||
} | ||
|
||
:focus-visible { | ||
border-radius: 0.125rem; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In several places where we want the focus ring to be rounded at the corners, I use this rule. I only apply the border-radius rule on |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,10 @@ | |
padding-right: 1rem; | ||
} | ||
|
||
:focus-visible { | ||
border-radius: 0.125rem; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want all of the focusable links and buttons in the header nav to have rounded corners, so I decided to use this blanket rule, but this means I have to override the border-radius for the search button. I thought about writing this rule as: :focus-visible:not(.search-button-field) {
border-radius: 0.125rem;
} But I noticed that this file has no other reference to anything search related. It's completely ignorant of the fact that it contains a search button, so I decided that it would be better to add an override rule in the search stylesheet rather than couple this stylesheet to the search button. Just to keep things separate. |
||
} | ||
|
||
// These items will define the height of the header | ||
.navbar-item { | ||
height: var(--pst-header-height); | ||
|
@@ -99,7 +103,6 @@ | |
color: var(--pst-color-text-muted); | ||
border: none; | ||
@include link-style-hover; | ||
@include focus-indicator; | ||
} | ||
|
||
.dropdown-menu { | ||
|
@@ -190,7 +193,6 @@ | |
} | ||
} | ||
@include icon-navbar-hover; | ||
@include focus-indicator; | ||
} | ||
|
||
// Hide the navbar header items on mobile because they're in the sidebar | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,11 @@ | |
font-size: var(--pst-sidebar-font-size); | ||
} | ||
|
||
// Focus styles | ||
:focus-visible { | ||
border-radius: 0.125rem; | ||
} | ||
|
||
// override bootstrap when navlink are displayed in the sidebar | ||
.nav-link { | ||
font-size: var(--pst-sidebar-font-size-mobile); | ||
|
@@ -80,6 +85,26 @@ | |
border: none; | ||
background-color: inherit; | ||
font-size: inherit; | ||
|
||
.dropdown-item { | ||
&:hover, | ||
&:focus { | ||
// In the mobile sidebar, the dropdown menu is inlined with the | ||
// other links, which do not have background-color changes on hover | ||
// and focus | ||
background-color: unset; | ||
} | ||
} | ||
} | ||
} | ||
|
||
.bd-navbar-elements { | ||
.nav-link { | ||
&:focus-visible { | ||
box-shadow: none; // Override Bootstrap | ||
outline: $focus-ring-outline; | ||
outline-offset: $focus-ring-width; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default focus ring for the links at the top of the mobile sidebar (under "Site Navigation") was hugging the edges too closely, so this rule just adds a gap, or offset. |
||
} | ||
} | ||
} | ||
|
||
|
@@ -93,7 +118,7 @@ | |
.sidebar-header-items__end { | ||
display: flex; | ||
align-items: center; | ||
gap: 0.5rem; | ||
gap: 1rem; | ||
} | ||
|
||
@include media-breakpoint-up($breakpoint-sidebar-primary) { | ||
|
@@ -171,8 +196,6 @@ | |
|
||
/* Between-page links and captions */ | ||
nav.bd-links { | ||
margin-right: -1rem; | ||
|
||
Comment on lines
-174
to
-175
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't figure out why this rule was here but it was clipping the focus ring, so I removed it. I asked @12rambau if he could remember why he added it and if I could remove it, and he replied, "I guess the only way to find out is to remove it 😄" |
||
@include media-breakpoint-up($breakpoint-sidebar-primary) { | ||
display: block; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,3 +22,16 @@ $dropdown-link-hover-bg: var(--pst-color-surface); | |
$dropdown-dark-link-hover-bg: var(--pst-color-surface); | ||
$dropdown-link-active-bg: var(--pst-color-surface); | ||
$dropdown-dark-link-active-bg: var(--pst-color-surface); | ||
|
||
$focus-ring-width: 0.1875rem; // 3px at 100% zoom (0.1875 * 16px base font = 3px) | ||
$focus-ring-opacity: 1; | ||
$focus-ring-color: var(--pst-color-accent); | ||
$focus-ring-blur: 0; | ||
$focus-ring-box-shadow: 0 0 $focus-ring-blur $focus-ring-width $focus-ring-color; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is how to customize the focus ring styles that Bootstrap provides. |
||
// outline creates the same style of focus ring, it just uses CSS outline instead of box shadow | ||
$focus-ring-outline: $focus-ring-color solid $focus-ring-width; | ||
$btn-focus-box-shadow: $focus-ring-box-shadow; | ||
|
||
.btn { | ||
--bs-btn-focus-box-shadow: #{$btn-focus-box-shadow}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,7 +50,7 @@ | |
|
||
{# A button hidden by default to help assistive devices quickly jump to main content #} | ||
{# ref: https://www.youtube.com/watch?v=VUR0I5mqq7I #} | ||
<a class="skip-link" href="#main-content">{{ _("Skip to main content") }}</a> | ||
<div class="skip-link"><a href="#main-content">{{ _("Skip to main content") }}</a></div> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I re-factored the skip link markup because if we use a single HTML tag then we have a focus ring that spans the entire width of the page, rather than just the link text. |
||
|
||
{%- endblock %} | ||
|
||
|
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.
Bootstrap uses box-shadow for focus rings, so I changed the code here to use
border-left
instead of using a box shadow to draw a vertical line or notch on the left side of the link in the TOC that corresponds to the current page that the user is on.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.
I think this caused a small glitch, because the border has an actual width compared to the box-shadow?
Comparing the readthedocs preview of this PR vs the latest docs:
https://pydata-sphinx-theme.readthedocs.io/en/latest/user_guide/layout.html#templates-and-components:
And on the PR (https://pydata-sphinx-theme--1549.org.readthedocs.build/en/1549/user_guide/layout.html#templates-and-components):
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.
ooh, good catch @jorisvandenbossche
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.
Yes, thanks for catching that! Follow up PR: #1561