search

pradyunsg / furo

A clean customizable documentation theme for Sphinx
https://pradyunsg.me/furo/quickstart
MIT License
2.65k stars 304 forks source link

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

Open Eric-Arellano opened 1 year ago

Eric-Arellano commented 1 year ago

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 commented 1 year ago

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: https://github.com/Qiskit/qiskit_sphinx_theme/issues/368#issuecomment-1584512877.

(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 commented 1 year ago

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 https://github.com/pradyunsg/furo/issues/546. But, totally fine with me if you don't want this change.

Eric-Arellano commented 8 months ago

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 commented 8 months ago

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.