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: version selector JS #1305

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
13 changes: 9 additions & 4 deletions src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ if (themeSwitchBtns.length) {
"list-group-item list-group-item-action py-1"
);
node.setAttribute("href", `${entry.url}${currentFilePath}`);
node.setAttribute("role", "option");
node.appendChild(span);

// on click, AJAX calls will check if the linked page exists before
Expand All @@ -345,21 +346,25 @@ if (themeSwitchBtns.length) {
node.dataset["versionName"] = entry.name;
node.dataset["version"] = entry.version;

document.querySelector(".version-switcher__menu").append(node);
// replace dropdown button text with the preferred display name of
// this version, rather than using sphinx's {{ version }} variable.
// also highlight the dropdown entry for the currently-viewed
// version's entry
if (
entry.version ==
"DOCUMENTATION_OPTIONS.version_switcher_version_match"
entry.version == DOCUMENTATION_OPTIONS.theme_switcher_version_match
) {
node.classList.add("active");
themeSwitchBtns.forEach((btn) => {
btn.innerText = btn.dataset["activeVersionName"] = entry.name;
btn.innerText = entry.name;
btn.dataset["activeVersionName"] = entry.name;
btn.dataset["activeVersion"] = entry.version;
});
}

const menus = document.querySelectorAll(".version-switcher__menu");
for (let m of menus) {
m.append(node.cloneNode(true));
}
});
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
<script>
document.write(`
<div class="version-switcher__container dropdown">
<button type="button" class="version-switcher__button btn btn-sm navbar-btn dropdown-toggle" data-bs-toggle="dropdown">
<button id="versionswitcherbutton" type="button" class="version-switcher__button btn btn-sm navbar-btn dropdown-toggle" data-bs-toggle="dropdown" aria-haspopup="listbox" aria-controls="versionswitcherlist" aria-label="Version switcher list">
{{ theme_switcher.get('version_match') }} <!-- this text may get changed later by javascript -->
<span class="caret"></span>
</button>
<div class="version-switcher__menu dropdown-menu list-group-flush py-0">
<div id="versionswitcherlist" class="version-switcher__menu dropdown-menu list-group-flush py-0" role="listbox" tabindex=-1>
<!-- dropdown will be populated by javascript on page load -->
</div>
</div>
Expand Down
17 changes: 16 additions & 1 deletion tests/test_a11y.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

# Using importorskip to ensure these tests are only loaded if Playwright is installed.
playwright = pytest.importorskip("playwright")
from playwright.sync_api import Page # noqa: E402
from playwright.sync_api import Page, expect # noqa: E402

# Important note: automated accessibility scans can only find a fraction of
# potential accessibility issues.
Expand Down Expand Up @@ -112,3 +112,18 @@ def test_axe_core_kitchen_sink(

# Expect Axe-core to have found 0 accessibility violations
assert len(results["violations"]) == 0, pretty_axe_results(results)


def test_version_switcher_highlighting(page: Page, url_base: str) -> None:
"""This isn't an a11y test, but needs a served site for Javascript to inject the version menu."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

@trallard In a follow-up PR I'd like to add a @requires_playwright decorator so that I can move this test out of test_a11y.py into test_build.py. Something similar to

https://github.com/mne-tools/mne-python/blob/68e4f89763135637110859ee0e05e594955815f1/mne/utils/_testing.py#L67-L81

OK with you if this non-a11y test squats in the test_a11y.py file for now until that gets done? (did it this way because of the importorskip issue; didn't want to importorskip the entire other tests file when only one test needed playwright)

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure, this makes sense to me

page.goto(url=url_base)
button = page.get_by_role("button", name="dev")
active_version_name = button.get_attribute("data-active-version-name")
entries = page.get_by_role("menuitem", include_hidden=True).filter(
has_text=active_version_name
)
assert (
entries.count() == 2
) # sidebar menu and topbar menu each have a matching entry
for entry in entries.all():
expect(entry).to_have_css("color", "rgb(69, 157, 185)")
6 changes: 4 additions & 2 deletions tests/test_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -646,12 +646,14 @@ def test_show_nav_level(sphinx_build_factory) -> None:
assert "checked" in checkbox.attrs


# the switcher files tested in test_version_switcher_error_states, not all of them exist
switcher_files = ["switcher.json", "http://a.b/switcher.json", "missing_url.json"]
"the switcher files tested in test_version_switcher, not all of them exist"


@pytest.mark.parametrize("url", switcher_files)
def test_version_switcher(sphinx_build_factory, file_regression, url) -> None:
def test_version_switcher_error_states(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is just a rename to make the test name more specific, since I'm adding another related test.

sphinx_build_factory, file_regression, url
) -> None:
"""Regression test the version switcher dropdown HTML.

Note that a lot of the switcher HTML gets populated by JavaScript,
Expand Down
4 changes: 2 additions & 2 deletions tests/test_build/navbar_switcher.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is used in our tests to make sure the version switcher component gets embedded correctly (prior to any JS adding the actual entries). Changes here simply mirror the changes to the template so that test still passes.

Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
<script>
document.write(`
<div class="version-switcher__container dropdown">
<button type="button" class="version-switcher__button btn btn-sm navbar-btn dropdown-toggle" data-bs-toggle="dropdown">
<button id="versionswitcherbutton" type="button" class="version-switcher__button btn btn-sm navbar-btn dropdown-toggle" data-bs-toggle="dropdown" aria-haspopup="listbox" aria-controls="versionswitcherlist" aria-label="Version switcher list">
0.7.1 <!-- this text may get changed later by javascript -->
<span class="caret"></span>
</button>
<div class="version-switcher__menu dropdown-menu list-group-flush py-0">
<div id="versionswitcherlist" class="version-switcher__menu dropdown-menu list-group-flush py-0" role="listbox" tabindex=-1>
<!-- dropdown will be populated by javascript on page load -->
</div>
</div>
Expand Down