Skip to content

Highlight correct TOC entry on page load #2185

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
Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Highlight correct TOC entry on page load #2185

wants to merge 8 commits into from

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented Apr 7, 2025 •
edited
Loading

Compare

Preview URL - Kitchen Sink - Admonitions - with #topic in the hash portion of the URL:
https://pydata-sphinx-theme--2185.org.readthedocs.build/en/2185/examples/kitchen-sink/admonitions.html#topic

The entry for topic in the right sidebar table of contents is highlighted, as shown in the following screenshot:

This pull request fixes the following bug, which is currently in the dev version of the theme / latest version of the docs.

Production URL - same page - #topic in hash:
https://pydata-sphinx-theme.readthedocs.io/en/latest/examples/kitchen-sink/admonitions.html#topic

The TOC entry for admonition (just below topic) is incorrectly highlighted, as shown in the following screenshot:

Caution

gabalafou
gabalafou commented Apr 7, 2025
View reviewed changes
Copy link
Collaborator Author

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

self review

src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js
@@ -1127,7 +1161,7 @@ function setupArticleTocSyncing() {
}

observer = new IntersectionObserver(callback, options);
headingsToTocLinks.keys().forEach((heading) => {
Array.from(headingsToTocLinks.keys()).forEach((heading) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was actually causing a runtime error in Safari for me. Only the latest versions of Safari support forEach on the map iterator.

@gabalafou
Copy link
Collaborator Author

Not exactly sure why tests are failing

@gabalafou
Copy link
Collaborator Author

Fails for me locally too, but also fails locally on main but not the same errors as what I'm seeing on the GitHub runner :-/

@trallard
Copy link
Collaborator

trallard commented Apr 7, 2025

The tests are failing on CI for the dev version of Sphinx (v8.3.0+/c13af9e) as this changes the behaviour of linkcheck_allowed_redirects (ref:https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-linkcheck_allowed_redirects).

A patch will be needed for this as the tests will keep failing in dev and later on when the new Sphinx release is out

@drammock
Copy link
Collaborator

drammock commented Apr 7, 2025

this changes the behaviour of linkcheck_allowed_redirects

xref to sphinx-doc/sphinx#13462

The problem with linkcheck appears to be an upstream bug

@gabalafou gabalafou added kind: bug Something isn't working impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship tag: javascript Pull requests that update Javascript code tag: UX Issues or improvements related to user experience labels Apr 7, 2025
@gabalafou
Copy link
Collaborator Author

Labeled this PR as block-release because it fixes a bug (in Safari, maybe other browsers too) that I was not aware of until I was working on this PR.

@gabalafou gabalafou mentioned this pull request Apr 25, 2025
Open
2 tasks
@gabalafou gabalafou linked an issue Apr 25, 2025 that may be closed by this pull request
BUG - Follow-up issues with new scrollspy #2178
Open
2 tasks
@gabalafou gabalafou mentioned this pull request Apr 25, 2025
Open
@stevepiercy stevepiercy mentioned this pull request May 1, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship kind: bug Something isn't working tag: javascript Pull requests that update Javascript code tag: UX Issues or improvements related to user experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG - Follow-up issues with new scrollspy
3 participants
@gabalafou @trallard @drammock