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

ENH: place the RDT flyout in the sidebar #1005

Merged
merged 16 commits into from
Oct 12, 2022
Merged

Conversation

12rambau
Copy link
Collaborator

@12rambau 12rambau commented Oct 10, 2022

Fix #194

in #705 @choldgraf was advocating in favor of moving the RTD's flyout in the header. I tried a lot and there really is no real estate available for it. we already added an option to limit the number of links displayed at the same time, the only place I could think about would be to replace the version switcher which could lead to lots of different issues.

In this PR I fall back on the initial idea to use the primary sidebar and embed the flyout in it at the bottom (we are advertising a "clean 3-column template" so let's use it!). This can also be considered a first step before moving it elsewhere.

dark light
fold dark-fold light-fold
expand dark-expand light-expand

@12rambau 12rambau marked this pull request as ready for review October 10, 2022 20:39
@12rambau 12rambau changed the title FIX: place the RDT flyout in the sidebar ENH: place the RDT flyout in the sidebar Oct 10, 2022
Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Only one thing I noticed: for me the switcher isn't visible until you scroll down two times. One to get to the bottom of the primary sidebar (and for some reason it's still hidden) and a second time to reveal the footer and the switcher.

I think if there's a switcher, it should always be visible in the sidebar, and the sidebar's contents should scroll above it.

As a future change, another option we could take is to make this switcher look like a self-container button that, when clicked, reveals the flyout menu. Then people could place it where they wish (in the header, in the sidebar, etc). It could have a similar size as our "version dropdown" button, but it would just have a more rich UI. I think to accomplish this, we'd only need to add the CSS to make sure that it didn't behave weirdly if the component were placed in the header, that's a bout it.

@12rambau
Copy link
Collaborator Author

As a future change, another option we could take is to make this switcher look like a self-container button that, when clicked, reveals the flyout menu. Then people could place it where they wish (in the header, in the sidebar, etc). It could have a similar size as our "version dropdown" button, but it would just have a more rich UI. I think to accomplish this, we'd only need to add the CSS to make sure that it didn't behave weirdly if the component were placed in the header, that's a bout it.

That's something I was thinking for every component, have a specific display depending on where it is added in the template, that's completely doable from a CSS point of view, just super long to code

@12rambau
Copy link
Collaborator Author

I think if there's a switcher, it should always be visible in the sidebar, and the sidebar's contents should scroll above it.

It's already the case the issue is the size of the sidebar itself. It's not taking into account the length of the announcement banner so it's 1 banner too high. The problem from a CSS point of view is that the banner can be bigger than 1 line. What would you think if we handle this via javascript? (maybe in another PR)

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to tweak the vertical position of the drawer in a follow-up...I'm +1 on this since it's a definite improvement over the "floating RTD menu".

@12rambau 12rambau merged commit adc8b1d into pydata:main Oct 12, 2022
@12rambau 12rambau deleted the readthedoc branch October 12, 2022 15:48
@jarrodmillman jarrodmillman added this to the 0.12 milestone Oct 17, 2022
@VeckoTheGecko
Copy link
Contributor

VeckoTheGecko commented Apr 2, 2023

Great feature!

To clarify, is the sidebar only possible to view on smaller screens? And if so, does that mean that if you're using RTD with this theme, that you need to double up; implementing and maintaining the PyData version switcher (for desktop users) as well as have the RTD switcher? Just wondering if there's a more elegant solution here that I'm missing (and if there is, I'm happy to submit a PR updating the RTD section of the docs).

EDIT: I realised that this is only an issue on pages with no section navigation

@12rambau
Copy link
Collaborator Author

12rambau commented Apr 2, 2023

@VeckoTheGecko yes it's only a problem for pages without navigation. A good workaround would be to code the a widget to put the RDT flyout in the header navbar (#705) but nobody started a PR yet.

I think the best way to proceed is to create a set of custom CSS for when the flyout handler is set in the navbar.

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.

ReadTheDocs version selector always on top for mobile displays
4 participants