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: Add support for ReadTheDocs popup #518

Merged
merged 14 commits into from
Mar 14, 2022

Conversation

choldgraf
Copy link
Member

@choldgraf choldgraf commented Mar 6, 2022

This adds some JS that "places" the ReadTheDocs popup in the sidebar so that it looks a bit nicer on page load. It creates a "top" and a "bottom" of our sidebar, so that things stay at the bottom regardless of the content at the top. Also adds a bunch of CSS to style the ReadTheDocs popup properly.

chrome_AdGNLQl6JI

@pradyunsg
Copy link
Member

FYI: I’m not able to see the injected button on an iPad in landscape mode.

@chrisjsewell
Copy link
Member

Cross-linking to: pradyunsg/furo#359 (was hoping @pradyunsg would do it, so then we could steal he's implementation 😅)

@chrisjsewell
Copy link
Member

For me (macbook, firefox), its only visible after scrolling down

@choldgraf
Copy link
Member Author

OK the latest commit cleaned things up a bit, but there's still one major UX issue that I think is related to both of the things you reported above:

The height of the sidebar container is 100vh. However, with the announcement bar at the top, it means that the sidebar extends slightly below the screen. This causes the RTD button to be hidden until somebody scrolls down enough to hide the announcement bar.

A better approach would be to somehow get the sidebar container to stretch to fill the remaining vertical space that fits within the screen, however much it is. However, I can't for the life of my figure out how to do it. I've tried align-self: stretch, but that doesn't seem to work, so I'm sort-of at a loss.

@chrisjsewell
Copy link
Member

chrisjsewell commented Mar 6, 2022

@choldgraf, it has actually been implemented in furo now 🎉 , did you look at that implementation? pradyunsg/furo#359 (reply in thread)

@pradyunsg
Copy link
Member

If it's in a flex container, flex-grow: 1 will make it fill the parent. I'm not sure if that's what you want tho. :)

@choldgraf
Copy link
Member Author

choldgraf commented Mar 6, 2022

Ya - I looked at the RTD implementation, though couldn't get the "template injection" method working, which is why this one uses a lightweight javascript approach.

I feel like the issue here is some kind of CSS bug that is probably not directly portable to the Furo theme :-/ I can have another look at how the sidebar behavior works though in case that helps illuminate things.

The weird thing is that on the "mobile view" of my browser, the RTD button shows up in the sidebar just fine, but when I try it with my actual phone, it is hidden below the screen. I dunno what's up with that.

@pradyunsg the issue w/ flex-grow is that the flex direction is row, not column, so flex-grow makes the sidebar take up more horizontal space, while I want it to take up all of the available vertical space.

@pradyunsg
Copy link
Member

Ah, the problem is the announcements.

I have the same problem on https://furo.readthedocs.io/en/latest/; so if you figure something out, lemme know. :P

@choldgraf
Copy link
Member Author

OK so at least on mobile I think that I've resolved the issue. Apparently vh units behave differently across mobile devices and it is less reliable in general, so working around that in the latest commit.

At this point the expected behavior is:

  • On widescreen, the RTD button should be visible by default
  • On widescreen with an announcement, it will be hidden until you scroll past the announcement
  • On mobile, it should be visible when you slide out the left drawer

The second bullet is definitely sub-optimal, but hopefully not a huge deal since announcements will likely be used relatively rarely, and if we can resolve #488 , then they can be dismissed on subsequent pageviews.

@choldgraf
Copy link
Member Author

(If anything here seems to be broken other than the announcement pushing down the RTD button, please let me know!)

@choldgraf
Copy link
Member Author

choldgraf commented Mar 9, 2022

Actually I think that I might have solved the issue of the footer floating to the bottom in executablebooks/sphinx-book-theme@ddd5d48 (#518) , we should double-check that nothing else unexpectedly changed.

@choldgraf
Copy link
Member Author

OK the latest push cleans up the CSS a bit so that it looks more in-line with how the other visual elements of the theme look. It is maybe a bit harder to spot, but I think that it looks a lot cleaner, and since it's in the same position as the RTD theme switcher I'd hope it won't be too confusing. I updated the top comment w/ a GIF of how it looks.

@choldgraf choldgraf merged commit 0473758 into executablebooks:master Mar 14, 2022
@choldgraf
Copy link
Member Author

ok gonna merge this one in and we can iterate on more improvements if there are issues that pop up!

@choldgraf choldgraf deleted the new-rtd-template branch March 14, 2022 23:44
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.

3 participants