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.

Sign up for GitHub

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

Jump to bottom

Fix More dropdown (html, aria, bootstrap classes) #1414

Merged
merged 9 commits into from
Aug 24, 2023
Merged

Fix More dropdown (html, aria, bootstrap classes) #1414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
aedc6a2
Fix More dropdown (html, aria, bootstrap classes)
gabalafou Aug 17, 2023
41f3b93
update dropdowns when user changes light/dark theme
gabalafou Aug 17, 2023
1d28b04
fix padding
gabalafou Aug 17, 2023
a7f307c
fix active, hover - colors, transition
gabalafou Aug 18, 2023
10dac40
Update src/pydata_sphinx_theme/toctree.py
gabalafou Aug 18, 2023
eed8141
lint
gabalafou Aug 18, 2023
9648d66
turn off all transitions on .nav-link
gabalafou Aug 22, 2023
13761bb
fix test
gabalafou Aug 23, 2023
00e2fae
make test more dry, readable
gabalafou Aug 24, 2023
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
9 changes: 9 additions & 0 deletions src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ function setTheme(mode) {
document.documentElement.dataset.mode = mode;
var theme = mode == "auto" ? colorScheme : mode;
document.documentElement.dataset.theme = theme;
// TODO: remove this line after Bootstrap upgrade
// v5.3 has a colors mode: https://getbootstrap.com/docs/5.3/customize/color-modes/
document.querySelectorAll(".dropdown-menu").forEach((el) => {
if (theme === "dark") {
el.classList.add("dropdown-menu-dark");
} else {
el.classList.remove("dropdown-menu-dark");
}
});

// save mode and theme
localStorage.setItem("mode", mode);
Expand Down
16 changes: 15 additions & 1 deletion src/pydata_sphinx_theme/assets/styles/sections/_header.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,21 @@
border: 1px solid var(--pst-color-border);
box-shadow: 0 0 0.3rem 0.1rem var(--pst-color-shadow);
background-color: var(--pst-color-on-background);
padding: 0.5rem 1rem;
padding: 0.5rem 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO we don't need the top/bottom padding either, but feel free to ignore this suggestion if you disagree

Suggested change
padding: 0.5rem 0;
padding: 0;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not that I agree or disagree. It's more that this PR doesn't exactly feel like the right place to change padding or override somebody else's prior aesthetic choice. The focus of this PR is about correcting (1) the AT (assistive tech, accessibility tree) semantics and (2) interaction mechanisms. Some of that necessarily entails changing some visual aspects of the UI, but padding is not one of those aspects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But I will gladly open up a follow-up PR to put it on the table :)

margin: 0.5rem 0;
min-width: 20rem;

.dropdown-item {
// Give the items in the dropdown some breathing room but let the hit
// and hover area of the items extend to the edges of the menu
padding: 0.25rem 1.5rem;

// Override Bootstrap
&:focus:not(:hover):not(:active) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to add the :not(:hover):not(:active), otherwise this focus rule overrides the Bootstrap-provided hover and active styles because the selector has higher specificity.

background-color: inherit;
}
}

// Hide the menu unless show has been clicked
&:not(.show) {
display: none;
Expand All @@ -130,6 +141,9 @@
.nav-link {
@include link-style-hover;

// Override Bootstrap
transition: none;

&.nav-external:after {
font: var(--fa-font-solid);
content: var(--pst-icon-external-link);
Expand Down
7 changes: 7 additions & 0 deletions src/pydata_sphinx_theme/assets/styles/variables/_bootstrap.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,10 @@ $grid-breakpoints: (
lg: 960px,
xl: 1200px,
);

$dropdown-link-hover-bg: var(--pst-color-surface);
// --pst-color-surface can also be assigned to the dark variant because it is
// scoped to different values depending on light/dark theme
$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);
Comment on lines +19 to +24
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if this is the right move, but it seemed that if we want to override the default dropdown styles in this dropdown, the we'd probably want to override them in any other dropdown that we might add in the future?

Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need separate light/dark variables --- oh, is it because these are preexisting bootstrap variables that we're just overriding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

correct

18 changes: 12 additions & 6 deletions src/pydata_sphinx_theme/toctree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,18 +137,24 @@ def generate_header_nav_html(n_links_before_dropdown: int = 5) -> str:
out = "\n".join(links_solo)

# Wrap the final few header items in a "more" dropdown
links_dropdown = links_html[n_links_before_dropdown:]
links_dropdown = [
# 🐲 brittle code, relies on the assumption that the code above
# gives each link in the nav a `nav-link` CSS class
html.replace("nav-link", "nav-link dropdown-item")
drammock marked this conversation as resolved.
Show resolved Hide resolved
for html in links_html[n_links_before_dropdown:]
]

if links_dropdown:
links_dropdown_html = "\n".join(links_dropdown)
out += f"""
<div class="nav-item dropdown">
<button class="btn dropdown-toggle nav-item" type="button" data-bs-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
<li class="nav-item dropdown">
drammock marked this conversation as resolved.
Show resolved Hide resolved
<button class="btn dropdown-toggle nav-item" type="button" data-bs-toggle="dropdown" aria-expanded="false" aria-controls="pst-header-nav-more-links">
More
</button>
<div class="dropdown-menu">
<ul id="pst-header-nav-more-links" class="dropdown-menu">
gabalafou marked this conversation as resolved.
Show resolved Hide resolved
drammock marked this conversation as resolved.
Show resolved Hide resolved
{links_dropdown_html}
</div>
</div>
</ul>
</li>
"""

return out
Expand Down
16 changes: 6 additions & 10 deletions tests/test_build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,21 +320,17 @@ def test_navbar_header_dropdown(sphinx_build_factory, n_links) -> None:
sphinx_build = sphinx_build_factory("base", confoverrides=confoverrides).build()
index_html = sphinx_build.html_tree("index.html")
navbar = index_html.select("ul.bd-navbar-elements")[0]
dropdowns = navbar.select("li.dropdown")
standalone_links = navbar.select(".navbar-nav > li.nav-item:not(.dropdown)")
if n_links == 0:
# There should be *only* a dropdown and no standalone links
assert navbar.select("div.dropdown") and not navbar.select(
".navbar-nav > li.nav-item"
)
assert len(dropdowns) == 1 and not standalone_links
if n_links == 4:
# There should be at least one standalone link, and a dropdown
assert navbar.select(".navbar-nav > li.nav-item") and navbar.select(
"div.dropdown"
)
# There should be `n_links` standalone links, and a dropdown
assert len(standalone_links) == n_links and len(dropdowns) == 1
if n_links == 8:
# There should be no dropdown and only standalone links
assert navbar.select(".navbar-nav > li.nav-item") and not navbar.select(
"div.dropdown"
)
assert standalone_links and not dropdowns


def test_sidebars_captions(sphinx_build_factory, file_regression) -> None:
Expand Down
Loading