Fix hit area icon links buttons #1866
Conversation
src/pydata_sphinx_theme/assets/styles/components/_icon-links.scss
Outdated
Show resolved
Hide resolved
| @@ -2,7 +2,30 @@ | |||
| * Icon links in the navbar | |||
| */ | |||
|
|
|||
| .navbar-icon-links { | |||
| .pst-navbar-icon { | |||
There was a problem hiding this comment.
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.
| @@ -169,7 +197,9 @@ | |||
| background-color: inherit; | |||
| border: none; | |||
|
|
|||
| @include icon-navbar-hover; | |||
| &:hover { | |||
| @include icon-hover; | |||
There was a problem hiding this comment.
Hmm. This is the only place using this mixin now. I should look into unifying.
| @@ -16,9 +16,9 @@ | |||
| <span><i class="{{ icon }} fa-lg" aria-hidden="true"></i></span> | |||
| <span class="sr-only">{{ name }}</span> | |||
| {%- elif type == "local" -%} | |||
| <img src="{{ pathto(icon, 1) }}" class="icon-link-image" alt="{{ name }}"/> | |||
| <span><img src="{{ pathto(icon, 1) }}" class="icon-link-image" alt="{{ name }}"/></span> | |||
There was a problem hiding this comment.
The span is needed for the line-height property, which was the easiest way I found to get the underline to appear in a consistent place across all header nav items (text and icon links).
I suppose I should copy paste the following comment wherever the span is used?
{# Use <span> for line-height #}
src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/version-switcher.html
Show resolved
Hide resolved
| border: none; | ||
|
|
||
| @include icon-navbar-hover; | ||
| padding: 0.5rem; |
There was a problem hiding this comment.
this is a substantive change (not just refactoring). Just flagging for other reviewers; I checked the PR build versus current main and I'm happy with the change (need to view in narrow viewport / on mobile to see the difference)
src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/version-switcher.html
Show resolved
Hide resolved
| .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 | ||
| } |
| {title} | ||
| </a> | ||
| </li> | ||
| """ | ||
| nav_item = "nav-item" | ||
| nav_link = "nav-link" | ||
| for link in links_data: | ||
| links_html.append( | ||
| dedent( | ||
| boilerplate.format( | ||
| active=" current active" if link.is_current else "", |
There was a problem hiding this comment.
| active=" current active" if link.is_current else "", | |
| active="current active" if link.is_current else "", |
Maybe ? The space is added unconditionally in the formatted string above.
| "pst-header-nav-item", | ||
| "", | ||
| ) | ||
| html.replace(nav_item, "").replace(nav_link, "nav-link dropdown-item") | ||
| for html in links_html[n_links_before_dropdown:] | ||
| ] |
There was a problem hiding this comment.
No need to do it in this PR, but after merge, maybe open an issue, we likely don't need to replace now that boilerplate have arguments. We can just slice links_html into solo and dropdown link and format differently.
|
Ok, let's get that in. |
Part of #1865.
Fixes external issue Quansight-Labs/czi-scientific-python-mgmt#94.
This pull request started as a simple increase of the hit area for the navbar icon links, but then I realized that with #1846 having been merged, I could do a little bit of code cleanup at the same time.