search

jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
177 stars 28 forks source link

"Edit this page" on RTD #304

Closed chrisjsewell closed 6 months ago

chrisjsewell commented 7 months ago

On https://jbms.github.io/sphinx-immaterial/customization.html#themeconf-edit_uri it says:

This is disabled for builds on ReadTheDocs as they implement their own mechanism based on the repository’s branch or tagged commit.

But I know the pydata-sphinx-theme (and its derivative sphinx-book-theme) have this functionality working on RTD: https://pydata-sphinx-theme.readthedocs.io/en/latest/user_guide/index.html

So are you sure this can't be enabled?

2bndy5 commented 7 months ago

It can be enabled. I just put a if statement to disabled it. If you want, we can change that. Simply remove the constant READTHEDOCS from this statement: https://github.com/jbms/sphinx-immaterial/blob/f7bbc8fd32f7ca958c9deb8c6e02738fb7382915/sphinx_immaterial/nav_adapt.py#L848

chrisjsewell commented 7 months ago

Yeh to me it seems unnecessary to disable, unless you have any reason to think differently?

2bndy5 commented 7 months ago

I just put a if statement to disabled it

Just to expand on this: The config for the edit-this-page button would be a bit more complicated when using the theme's version selector drop down. I figured RTD's implementation was superior because it automatically accounts for branch or tag. I didn't think to compare with other themes.

2bndy5 commented 7 months ago

No objections to enabling. PR is welcome, but the docs should be adjusted accordingly.

chrisjsewell commented 7 months ago

Ah ok so there is a bit of an orthogonal issue here: I see in https://sphinx-immaterial.readthedocs.io/en/latest/ the RTD dropdown shows up (top-right), but in the sphinx-needs documentation it was removed via CSS (https://github.com/useblocks/sphinx-needs/blob/3486ac383ea2ba1a3f65c2484c84b69647e119c4/docs/_static/custom.css#L157)

Probably it was removed, because currently it does not look so nice where it shows up. In furo (see e.g. https://sphinx-design.readthedocs.io/en/furo-theme/) and https://sphinx-book-theme.readthedocs.io I know there has been HTML/CSS added, to make the RTD drop-down integrate nicer with the theme

you see its down on the bottom left in the ToC section

image
2bndy5 commented 7 months ago

Yeah, I chose to make it float above the right ToC because it wouldn't require altering any HTML templates and seemed the most non intrusive position (unless the header nav bar is hidden). Unfortunately, altering the HTML templates is a stricter undesirable change for us as it complicates merging updates from the mkdocs-material src.

The left side ToC is only visible in this theme when the browser veiwport is sufficiently wide. Moving the RTD menu into the global ToC (on left side) would require changing the HTML templates in more than 1 place, a rather significant divergence from upstream's src.

2bndy5 commented 7 months ago

BTW, the RTD menu for that pydata-theme on mobile still floats in lower-right corner (the original default position) and obscures the main body of text (as well as the scroll-to-top button): Screenshot_20231116-120528_Firefox.jpg

On mobile, this theme can be inadvertently forced to use a wider width than offered by the device's native breakpoint, but at least the floating RTD menu (& scroll-to-top button) is less obstructive: Screenshot_20231116-121106_Firefox.jpg

2bndy5 commented 7 months ago

I suppose we could create our own RTD HTML template and conditionally include it in the navbar, but this is getting off topic...

Looks like furo theme hosts its own RTD drop-down HTML src (and duplicated with a different filename for some reason) which is included in Sphinx' sidebar option of theme.conf.

Unfortunately, this theme doesn't respect Sphinx' sidebar theme.conf option or the html_sidebars config option (as far as I know), so we'd have to adjust the original/upstream HTML template in some way.

In a Firefox dev console, I was able to get what might be equivalent to the already available version selector by moving the RTD floating "badge" to the end of the header navbar element. With some more CSS tweaking I got it to look like this: image and with mobile breakpoint: image However, The RTD extension uses a statically hosted JS (see minified src here). So, the native JS solution from RTD makes the header bar height expand when the RTD menu is opened: image which might be fixable with the new HTML template (unlikely due to difference in DOM) or by hosting our own RTD JS to toggle the expanded menu (tech debt due to bundling our JS at pkg deployment).

I'm not sure what the preferred solution is here, but after researching how this is done in furo, I'm still leaning towards "don't fix what ain't broken."

2bndy5 commented 7 months ago

@chrisjsewell I opened #306 as a proof of concept in moving the RTD flyout menu to the header navbar (previewed on RTD here). Its still not very ideal on mobile, but it is a WIP. The injected RTD "badge" is not hidden on purpose (for comparison purposes).

The JS that RTD uses seems to inject the badge in a way that isn't great for themes that aren't rtd-sphinx-theme. Even the advice in RTD's docs is really meant for variants of rtd-sphinx-theme (not really useful for any other themes). Although, they're currently revising the process of integrating the flyout menu...