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

Fix hit area icon links buttons #1866

Merged
merged 6 commits into from
Jun 14, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 42 additions & 97 deletions src/pydata_sphinx_theme/assets/styles/abstracts/_links.scss
Original file line number Diff line number Diff line change
Expand Up @@ -164,114 +164,59 @@ $link-hover-decoration-thickness: string.unquote(
}
}

// Navigation bar current page link styles
// ---------------------------------------
// Adds a bottom underline, this leaves enough space for the hover state without
// cluttering the navbar.
// We want the side box shadow to have the same thickness as the hover underline
@mixin link-navbar-current {
Carreau marked this conversation as resolved.
Show resolved Hide resolved
font-weight: 600;
color: var(--pst-color-primary);
// Heaver navbar text and icon links
gabalafou marked this conversation as resolved.
Show resolved Hide resolved
// ---------------------------------
// (includes light/dark mode button)

// This mixin makes it possible to show hover/underline and focus/ring styles at
// the same time. The trick is to use:
// - a pseudo-element with bottom border for the hover underline
// - a CSS outline for the focus ring.

// Normally we use box-shadow for underline and outline for focus ring. But we
// cannot apply box-shadow and outline together on the same element because the
// border-radius value that we use to round the outline will also round the
// box-shadow used for the underline. We also cannot use text-underline because
// it does not work on non-text links, nor do we want to use it on text links
// that we want to treat as blocks, such as the header nav links because the
// underline will wrap across two lines if the link text also wraps across two
// lines.
@mixin link-style-block {
color: var(--pst-color-text-muted);

@if $link-hover-decoration-thickness {
border-bottom: $link-hover-decoration-thickness
solid
var(--pst-color-primary);
}
}
// Set position relative so that the child ::before pseudo-element's absolute
// position is relative to this element.
position: relative;

// Navigation bar icon links hover styles
// --------------------------------------
// Adds a bottom box-shadow - since there is no text we cannot use text-decoration
// We want the side box shadow to have the same thickness as the hover underline
@mixin icon-navbar-hover {
Carreau marked this conversation as resolved.
Show resolved Hide resolved
&:hover {
color: var(--pst-color-link-hover);
// Set up pseudo-element used for hover underline styles
&::before {
content: "";
display: block;
position: absolute;
inset: 0;
background-color: transparent;

@if $link-hover-decoration-thickness {
box-shadow: 0
$link-hover-decoration-thickness
0
var(--pst-color-link-hover);
bottom: calc(-1 * $link-hover-decoration-thickness);
margin: $link-hover-decoration-thickness 0;
}
}
}

// Mixin for links in the header (and the More dropdown toggle).

// The mixin assumes it will be applied to some element X with a markup structure
// like: X > .nav-link, or X > .dropdown-toggle.

// It also assumes X.current is how the app annotates which item in the header nav
// corresponds to the section in the docs that the user is currently reading.
@mixin header-link {
Carreau marked this conversation as resolved.
Show resolved Hide resolved
// Target the child and not the parent because we want the underline in the
// mobile sidebar to only span the width of the text not the entire row/line.
> .nav-link,
> .dropdown-toggle {
border-radius: 2px;
color: var(--pst-color-text-muted);
}

> .nav-link {
// Set up pseudo-element for hover and current states below.
position: relative;

&:hover {
color: var(--pst-color-secondary);
text-decoration: none; // override the link-style-hover mixin
&::before {
content: "";
display: block;
position: absolute;
inset: 0;
background-color: transparent;
}

// Underline on hover.
// - Don't use text-decoration because it will wrap across two lines if
// the link text also wraps across two lines.
// - Use pseudo-element in order to avoid the border-radius values
// rounding the edges of the underline. (And since a header link can be
// both focused and hovered at the same time and we want the focus ring
// but not the underline to be rounded, we cannot use a box shadow or
// bottom border link element to create the underline, or else it will
// be rounded and if we apply border-radius 0 then the hovered focus
// ring would go from rounded to sharp. So we have to use the
// pseudo-element.)
&:hover {
color: var(--pst-color-secondary);
text-decoration: none; // override the link-style-hover mixin
&::before {
border-bottom: 3px solid var(--pst-color-secondary);
@if $link-hover-decoration-thickness {
border-bottom: $link-hover-decoration-thickness
solid
var(--pst-color-secondary);
}
}

&:focus-visible {
box-shadow: none; // override Bootstrap
outline: 3px solid var(--pst-color-accent);
outline-offset: 3px;
}
}

> .dropdown-toggle {
&:focus-visible {
box-shadow: $focus-ring-box-shadow;
}

&:hover {
text-decoration: none;
box-shadow: 0 0 0 $focus-ring-width var(--pst-color-link-hover); // purple focus ring
// Brighten the text on hover (muted -> base)
color: var(--pst-color-text-base);
}
}

&.current {
> .nav-link {
color: var(--pst-color-primary);

// Underline the current navbar item
&::before {
border-bottom: 3px solid var(--pst-color-primary);
}
}
&:focus-visible {
box-shadow: none; // override Bootstrap
outline: 3px solid var(--pst-color-accent);
outline-offset: 3px;
}
}
11 changes: 11 additions & 0 deletions src/pydata_sphinx_theme/assets/styles/abstracts/_mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,14 @@
}
}
}

// Minimum mouse hit area
// ----------------------
// Ensures that the element has a minimum hit area that conforms to
// accessibility guidelines. For WCAG AA, we need 24px x 24px, see:
// https://www.w3.org/WAI/WCAG22/Understanding/target-size-minimum.html
@mixin min-hit-area() {
box-sizing: border-box;
min-width: 24px;
min-height: 24px;
}
2 changes: 1 addition & 1 deletion src/pydata_sphinx_theme/assets/styles/base/_base.scss
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ body {
background-color: var(--pst-color-background);
font-family: var(--pst-font-family-base);
font-weight: 400;
line-height: 1.65;
line-height: $line-height-body;
color: var(--pst-color-text-base);
min-height: 100vh;
display: flex;
Expand Down
42 changes: 23 additions & 19 deletions src/pydata_sphinx_theme/assets/styles/components/_icon-links.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,29 @@
* Icon links in the navbar
*/

.navbar-icon-links {
.pst-navbar-icon {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that the navbar-btn class isn't defined by Bootstrap but we weren't using it, so I decided to delete navbar-btn everwhere and instead create a new class, pst-navbar-icon which I use to apply consistent styling to all the of the icon links and buttons.

// Extra specificity needed for overrides
html & {
@include min-hit-area;
@include link-style-block;

display: flex;
align-items: center;
justify-content: center;

// Bootstrap overrides
border-radius: 0;
border: none;
font-size: 1rem;
line-height: $line-height-body; // Override Boostrap, which defines a separate line-height for buttons
padding: $navbar-link-padding-y 0; // Horizontal white space in nav bar between items is controlled via column gap rule on the container.

// Make the navbar icon links have the same size as the navbar text links
height: calc(2 * $navbar-link-padding-y + $line-height-body * 1rem);
}
}

ul.navbar-icon-links {
display: flex;
flex-flow: row wrap;
column-gap: 1rem;
Expand All @@ -12,24 +34,6 @@
margin-bottom: 0;
list-style: none;

// Remove the padding so that we can define it with flexbox gap above
li.nav-item a.nav-link {
padding-left: 0;
padding-right: 0;

@include icon-navbar-hover;

&:focus {
color: inherit;
Carreau marked this conversation as resolved.
Show resolved Hide resolved
}
}

// Spacing and centering
a span {
display: flex;
align-items: center;
}

// Icons styling
i {
&.fa-brands,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,10 @@
/**
* Navigation links in the navbar and icon links
*/
.navbar-nav,
.navbar-icon-links {
Carreau marked this conversation as resolved.
Show resolved Hide resolved
ul.navbar-nav {
Carreau marked this conversation as resolved.
Show resolved Hide resolved
// Reduce padding of nested `ul` items a bit
ul {
display: block;
list-style: none;

// Reduce padding of nested `ul` items a bit
ul {
padding: 0 0 0 1rem;
}
padding: 0 0 0 1rem;
}

// Navbar links - do not have an underline by default
Expand All @@ -22,8 +16,8 @@
display: flex;
align-items: center;
height: 100%;
padding-top: 0.25rem;
padding-bottom: 0.25rem;
padding-top: $navbar-link-padding-y;
padding-bottom: $navbar-link-padding-y;

@include link-style-text;
}
Expand Down
22 changes: 2 additions & 20 deletions src/pydata_sphinx_theme/assets/styles/components/_search.scss
Original file line number Diff line number Diff line change
Expand Up @@ -67,26 +67,8 @@
*/

// Search link icon should be a bit bigger since it is separate from icon links
.search-button {
display: flex;
align-items: center;
align-content: center;
color: var(--pst-color-text-muted);
padding: 0;
border-radius: 0;
border: none; // Override Bootstrap button border
font-size: 1rem; // Override Bootstrap 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;

@include icon-navbar-hover;
Carreau marked this conversation as resolved.
Show resolved Hide resolved

i {
font-size: 1.3rem;
}
.search-button i {
font-size: 1.3rem;
}

// __search-container will only show up when we use the search pop-up bar
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,7 @@
*/

.theme-switch-button {
color: var(--pst-color-text-muted);
border-radius: 0;
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;
}
Carreau marked this conversation as resolved.
Show resolved Hide resolved

span {
.theme-switch {
display: none;

&:active {
Expand All @@ -31,14 +17,10 @@
}
}

html[data-mode="auto"] .theme-switch-button span[data-mode="auto"] {
display: flex;
}

html[data-mode="light"] .theme-switch-button span[data-mode="light"] {
display: flex;
}

html[data-mode="dark"] .theme-switch-button span[data-mode="dark"] {
display: flex;
@each $mode in auto, light, dark {
html[data-mode="#{$mode}"]
.theme-switch-button
.theme-switch[data-mode="#{$mode}"] {
display: inline; // inline needed for span height to be calculated using inherited font size and line height
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ button.version-switcher__button,
font-size: 1.1em; // A bit smaller than other menu font
z-index: $zindex-modal; // higher than the sidebars

// Make sure it meets WCAG target size requirement no matter the version
// string displayed in the button
@include min-hit-area;
Carreau marked this conversation as resolved.
Show resolved Hide resolved

@include media-breakpoint-up($breakpoint-sidebar-primary) {
font-size: unset;
}
Expand Down
Loading
Loading