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

Make current page section detection resilient to sticky elements above header #664

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Eric-Arellano
Copy link
Contributor

Problem

In https://github.com/Qiskit/qiskit_sphinx_theme, we have to add a custom header to the top of the site, like this:

Screenshot 2023-06-09 at 6 31 10 AM

This naturally breaks the detection of what the current scroll section is in the page table of contents, as it does not account for elements above the header:

Solution

This is easily fixed by also including the header.top in the offset.

In base Furo, header.top is 0, so this change has no impact. I suspected this PR might be necessary for setting up banners, but because banners are not sticky, header.top ends up going back to 0 even with a banner when scrolling down.

While it has no impact on base Furo, it makes customization of Furo much easier. It's particularly complicated to tweak the JS code because it's being built by Webpack.

@Eric-Arellano
Copy link
Contributor Author

While it has no impact on base Furo, it makes customization of Furo much easier. It's particularly complicated to tweak the JS code because it's being built by Webpack.

If you're willing to approve this PR, I'd hugely appreciate it. I think this is how we'd have to end up modifying pre-existing JS: Qiskit/qiskit_sphinx_theme#368 (comment).

(We can't use the amazing sphinx-theme-builder because we have a non-standard setup of distributing multiple themes in the same package.)

Thanks again for your work on Furo!

@Eric-Arellano
Copy link
Contributor Author

Eric-Arellano commented Jul 3, 2023

Ah, bummer. Turns out this won't actually help qiskit-sphinx-theme because our top nav bar is not changing the top for Furo's header element. So, qiskit-sphinx-theme will always need to fork Furo's JavaScript. (We recently switched to sphinx-theme-builder, so it's now viable - great work with that project!)

This PR may still be helpful to other projects that add a sticky top element and for #546. But, totally fine with me if you don't want this change.

@Eric-Arellano
Copy link
Contributor Author

Gentle bump. As explained above, this doesn't actually help qiskit-sphinx-theme, but I think it may still be valuable to others.

No worries if you prefer to close :)

@pradyunsg
Copy link
Owner

Thanks for the bump -- I'll likely spend some more time on this theme during my vacation; have had a lot of life happening lately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants