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

"Edit this page" on RTD #304

Closed
chrisjsewell opened this issue Nov 16, 2023 · 9 comments · Fixed by #305
Closed

"Edit this page" on RTD #304

chrisjsewell opened this issue Nov 16, 2023 · 9 comments · Fixed by #305

Comments

@chrisjsewell
Copy link

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
Copy link
Collaborator

2bndy5 commented Nov 16, 2023

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:

if repo_url and edit_uri and not READTHEDOCS and "hide-edit-link" not in meta:

@chrisjsewell
Copy link
Author

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

@2bndy5
Copy link
Collaborator

2bndy5 commented Nov 16, 2023

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
Copy link
Collaborator

2bndy5 commented Nov 16, 2023

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

@chrisjsewell
Copy link
Author

chrisjsewell commented Nov 16, 2023

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
Copy link
Collaborator

2bndy5 commented Nov 16, 2023

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
Copy link
Collaborator

2bndy5 commented Nov 16, 2023

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
Copy link
Collaborator

2bndy5 commented Nov 16, 2023

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
Copy link
Collaborator

2bndy5 commented Nov 18, 2023

@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...

2bndy5 added a commit that referenced this issue Dec 12, 2023
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 a pull request may close this issue.

2 participants