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 flyout menu scrollable #7375

Closed
wants to merge 2 commits into from
Closed

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Aug 10, 2020

I have a project on RTD which has a lot of branches and tags.

When the RTD theme is used, the "version selection area" in the sidebar is scrollable if the window height is too small to accommodate all those links; see e.g. the page https://nbsphinx.readthedocs.io/en/rtd-theme/ (you probably have to change the height of the browser window to see this).

When a different Sphinx theme is used, the "version selection area" is in a "badge" in the bottom right corner of the page. When this "badge" is opened and the window height is too small, there is no scrolling and the top part of the "version selection" is not visible, see e.g. https://nbsphinx.readthedocs.io/en/alabaster-theme/.

This PR tries to make this scrollable.

I'm not sure whether media/css/readthedocs-doc-embed.css is the right place to put this. readthedocs/core/static/core/css/badge_only.css seems to be an alternative location, but I don't know how this is generated.

Maybe there is a much better way to do this. Please let me know any suggestions for improvements.

@stsewd
Copy link
Member

stsewd commented Aug 10, 2020

Thanks, I think this file is the right place to put that. You may want to take a look at hidden versions too https://docs.readthedocs.io/en/stable/versions.html#version-states

@stsewd stsewd requested a review from agjohnson August 10, 2020 18:06
@mgeier
Copy link
Contributor Author

mgeier commented Aug 10, 2020

@stsewd I don't have experience with hidden versions, how do they affect the "badge"?

I just tried a hidden version and I don't see any difference (except for the hidden version not being listed in the list of non-hidden versions).

Or do you mean something else?

@stsewd
Copy link
Member

stsewd commented Aug 10, 2020

I just tried a hidden version and I don't see any difference (except for the hidden version not being listed in the list of non-hidden versions).

That's the feature yes. This way you don't need to list all those branches that aren't real versions and pollute the flyout menu.

@mgeier
Copy link
Contributor Author

mgeier commented Aug 10, 2020

Ah, now I understand!

You wanted to help me get rid of those versions ... but I don't want to!
I want to have every single one of them.

And I don't think I'm the only one. I've definitely seen other projects with many versions (but as far as I recall, those were using the RTD theme, so scrolling was available for them).

@stsewd stsewd changed the title Make "badge" scrollable Make flyout menu scrollable Aug 10, 2020
@mgeier
Copy link
Contributor Author

mgeier commented Aug 25, 2020

I think it would be even better if the top part (where you click to close the flyout menu) would stay visible: 7ad8141

@mgeier
Copy link
Contributor Author

mgeier commented Oct 5, 2020

Any opinions on this PR?
Can this please be reviewed?

@mgeier
Copy link
Contributor Author

mgeier commented Nov 16, 2020

Ping?

@stale
Copy link

stale bot commented Dec 31, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Dec 31, 2020
@mgeier
Copy link
Contributor Author

mgeier commented Jan 1, 2021

Dear bot, can you please not close this?

@stale stale bot removed the Status: stale Issue will be considered inactive soon label Jan 1, 2021
@stsewd stsewd added the Accepted Accepted issue on our roadmap label Jan 4, 2021
@mgeier
Copy link
Contributor Author

mgeier commented Jan 25, 2021

Any news on this?

It has the "Accepted" label but no approving review yet ...?

@stsewd
Copy link
Member

stsewd commented Jan 25, 2021

Sorry, the team with css knowledge are busy with the site re-design, so it may take a while before reviewing this change.

@mgeier
Copy link
Contributor Author

mgeier commented Jan 28, 2021

Thanks for the update!
I'm looking forward to the new site design!

If this is handled by a separate team, is there a special label or something to mark this PR with?

Anyway, I'll just keep pinging this in more or less regular intervals ...

@stsewd
Copy link
Member

stsewd commented Jan 28, 2021

I've already requested a review from there.

@humitos
Copy link
Member

humitos commented Mar 30, 2021

Just an update here. We are re-designing the flyout and I thought it would be useful to link it to that document here: #8052

@mgeier
Copy link
Contributor Author

mgeier commented Apr 27, 2021

Is there a timeline for #8052?

This PR is a really small change, what about applying it earlier?

It looks like #8052 might take a bit longer, but in the end either way the flyout menu should be scrollable, shouldn't it?

And #8052 doesn't really seem to be about CSS, or is it?

@stsewd
Copy link
Member

stsewd commented Apr 27, 2021

They are related, but not necessary for this change.

@mgeier
Copy link
Contributor Author

mgeier commented Jul 8, 2021

Ping?

@mgeier
Copy link
Contributor Author

mgeier commented May 1, 2022

Any news on this?

@ericholscher
Copy link
Member

@agjohnson Would be good to take a look. This does seem like a pretty simple change, so we should 👍 or 👎 it.

@agjohnson
Copy link
Contributor

I'll do a quick test of this, it seems small enough. However:

I'm not sure whether media/css/readthedocs-doc-embed.css is the right place to put this. readthedocs/core/static/core/css/badge_only.css seems to be an alternative location, but I don't know how this is generated.

We don't make this clear at all, but the badge_only.css actually comes from the sphinx_rtd_theme. We copy that file into this repo manually. I've raised breaking this out because it's a really weird authoring and packaging pattern. Alas, these edits do belong there instead, but we could just merge this and backport this to the theme too.

readthedocs-doc-embed.css has a number of hacks that we put in place instead of releasing sphinx_rtd_theme, which feels like a pattern to avoid, generally speaking.

@mgeier
Copy link
Contributor Author

mgeier commented Jun 4, 2022

We don't make this clear at all, but the badge_only.css actually comes from the sphinx_rtd_theme.

And even more confusing, this is only used when the RTD theme is not used, so I would expect it anywhere except in the RTD theme repo.

Alas, these edits do belong there instead, but we could just merge this and backport this to the theme too.

I've tried implementing it there, but I don't know if that works: readthedocs/sphinx_rtd_theme#1297

@agjohnson
Copy link
Contributor

And even more confusing, this is only used when the RTD theme is not used, so I would expect it anywhere except in the RTD theme repo.

👍 I've brought this up as well. I'd like to split this out to it's own repository as well. Currently, it's in an odd spot, as it is using the same underlying styles as the sphinx_rtd_theme flyout

I've tried implementing it there, but I don't know if that works: readthedocs/sphinx_rtd_theme#1297

I started down this path as well, but was having trouble testing it. I hit a block with my local application instance in addition, so had trouble testing anything here. I'll try to return to these PRs this week for testing.

@humitos
Copy link
Member

humitos commented Aug 9, 2023

I'm closing this PR. We are working on a complete re-design of the flyout at readthedocs/addons#70

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

Successfully merging this pull request may close these issues.

5 participants