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

Fix RTD version switcher hiding #1398

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Conversation

drammock
Copy link
Collaborator

@drammock drammock commented Aug 2, 2023

(supercedes) closes #705
closes #1092

WIP to fix the problem of the RTD version switcher disappearing on pages where the left sidebar is suppressed. The solution here is to piggyback on the theme's built-in version switcher, and just place the RTD switcher wherever the theme switcher was requested to go. Let's see if it works, if it does I'll update docs etc accordingly.

@drammock drammock changed the title proof of concept Fix RTD version switcher hiding Aug 2, 2023
@drammock drammock force-pushed the rtd-flyout branch 14 times, most recently from 9f7348f to 65daf50 Compare August 2, 2023 22:32
@drammock drammock force-pushed the rtd-flyout branch 4 times, most recently from 7c943bf to 49ceb7b Compare August 3, 2023 22:03
@drammock drammock force-pushed the rtd-flyout branch 3 times, most recently from 1483788 to 16ea4a9 Compare August 4, 2023 20:35
@drammock drammock force-pushed the rtd-flyout branch 2 times, most recently from 30e9360 to c9589bc Compare August 5, 2023 15:23
@drammock
Copy link
Collaborator Author

drammock commented Aug 5, 2023

hey @12rambau this is getting pretty close:

Screenshot_2023-08-05_12-00-07

maybe you can help with the last mile? Here's what remains:

  • needs documentation, I'll do that (have a local draft already)
  • test failures you can ignore, I'll fix them; they are because on this PR I'm embedding the RTD flyout instead of our own version switcher, so the file regression test fails.
  • the RTD flyout is not getting copied to the narrow-screen sidebar position. I tried to hack your MutationObserver code so that when RTD injects the flyout, it gets cloned, and then the onclick handler gets recreated (JS node cloning never copies event handlers). AFAICT the mutation observer is never even running (based on console.log entries not showing up). Any idea what is going wrong there?

@gabalafou
Copy link
Collaborator

May I ask a perhaps controversial question: do we really need the Read The Docs version switcher?

@drammock
Copy link
Collaborator Author

May I ask a perhaps controversial question: do we really need the Read The Docs version switcher?

It does more stuff than our in-built one does, and some users seem to want / prefer it. 🤷🏻 One solid advantage it has for RTD users is that it is config-free (no JSON file to keep updated) as it gets info about all the versions from the user's RTD account config / history.

Note also that RTD is testing out a rewritten version of their flyout which this PR won't handle... but it's an opt-in beta addon at the moment, and (I think?) RTD will continue to support the old version for quite some time, so if we can figure out the last missing bits here I still think this is a worthwhile addition to PST.

@humitos
Copy link

humitos commented Feb 28, 2024

Note also that RTD is testing out a rewritten version of their flyout which this PR won't handle... but it's an opt-in beta addon at the moment

Hi @drammock 👋🏼 . I'm one of the Read the Docs developers working on the new addons implementation for the flyout. The new implementation won't use a HTML blob that themes will need to inject as-is, but instead, it will expose all the data as a JSON object via a JavaScript CustomEvent where the theme could subscribe to render the flyout when that data is ready.

I wrote a POC for this implementation in the Read the Docs Sphinx's theme at readthedocs/sphinx_rtd_theme#1526. It would be good if you can take a look at it and provide some feedback. I'll be happy to receive it 🙏🏼

RTD will continue to support the old version for quite some time, so if we can figure out the last missing bits here I still think this is a worthwhile addition to PST.

We don't plan to deprecate the old flyout immediately, but we will stop promoting it and promote more projects migrating to the new addons because it will have a lot more features and it will be easy to maintain and customize.

@trallard
Copy link
Collaborator

Hey, @humitos - sniping here - as we have been auditing and remediating accessibility here, we have noticed some accessibility improvements needed in the RTD flyout (color contrast, keyboard navigation, and such). Where is the best place to pass this feedback? Is the linked POC ok? or would it be best to create a whole new issue?

@humitos
Copy link

humitos commented Feb 29, 2024

@trallard hi 👋🏼

Where is the best place to pass this feedback? Is the linked POC ok? or would it be best to create a whole new issue?

The work for the "generic" implementation of the new addons flyout lives in this repository https://github.com/readthedocs/addons so you can create an issue there.

However, if you are talking about the "integration of the flyout in the Read the Docs Sphinx theme", you can open an issue in the Sphinx's theme at https://github.com/readthedocs/sphinx_rtd_theme.

If you think that both have these accessibility issues, you can open an issue on both 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants