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

Bump to dev version + update versions in switcher.json #574

Merged
merged 5 commits into from
Feb 25, 2022

Conversation

jorisvandenbossche
Copy link
Member

@jorisvandenbossche jorisvandenbossche commented Jan 19, 2022

Related to the discussion in #556, and trying to improve the versions in the dropdown in our own demo docs.

cc @drammock

  • I added stable and latest versions, and therefore needed to remove the harcoded "v" in the template url
  • I trimmed down the list of versions a bit

@damianavila
Copy link
Collaborator

Highlighting this comment from another PR: #576 (comment)

@choldgraf
Copy link
Collaborator

I rebased on master and pushed a commit that:

  • Added a check for the READTHEDOCS_VERSION variable, and uses it if found
  • Added extra_classes for stable and dev to see if this works

@damianavila
Copy link
Collaborator

Added extra_classes for stable and dev to see if this works

When I check for the class on the RTD deployment of this PR, I do not see "dev" the extra class...

Screen Shot 2022-02-24 at 10 25 56

@choldgraf, can you confirm this is the case for you as well?

@damianavila
Copy link
Collaborator

@tupui, since you implemented #576, do you have any thoughts?

@tupui
Copy link
Contributor

tupui commented Feb 24, 2022

But the json is not deployed. This is the issue no? Or are you checking by manually pointing to this PR's json?

@damianavila
Copy link
Collaborator

But the json is not deployed.

You are right... I forgot about that 😉
This is in fact confusing if you have a weak memory like me... I think we might find a way to actually "see" changes on the version-switcher-related stuff on RTD previews, but that is another discussion!

@jorisvandenbossche
Copy link
Member Author

Thanks for the updates! Since we will only really see how it goes once merged, let's merge this

@jorisvandenbossche jorisvandenbossche merged commit 0c77b6d into pydata:master Feb 25, 2022
@jorisvandenbossche jorisvandenbossche deleted the update-version branch February 25, 2022 19:34
@jorisvandenbossche
Copy link
Member Author

Hmm, so the dropdown is not working. But that might actually already be an issue with #576?

It's giving an error in the console: Uncaught TypeError: $(...).classList is undefined, which is pointing to

$("#version_switcher_menu").classList.add(...entry.extra_classes);

@tupui
Copy link
Contributor

tupui commented Feb 25, 2022

@jorisvandenbossche sorry about that. But please see #597 as @choldgraf was changing the logic to not rely on JS here.

@jorisvandenbossche
Copy link
Member Author

No problem. And yes, I saw that PR, but I thought this could go in first, so we could ensure #597 would ensure the same behaviour ..

@tupui
Copy link
Contributor

tupui commented Feb 25, 2022

Got it, sorry cannot really help a lot here. I got help for the syntax at the end. I originally had a for loop and I recall testing it locally and I believe it was working.

@damianavila
Copy link
Collaborator

damianavila commented Feb 25, 2022

$("#version_switcher_menu").classList.add(...entry.extra_classes);

You might need to get the actual node with jquery... I think something like the following (untested) might work?

$("#version_switcher_menu")[0].classList.add(...entry.extra_classes);

@damianavila
Copy link
Collaborator

damianavila commented Feb 26, 2022

OK, here you have a PR #599 fixing this one among other things I have talked about in my above comments (hopefully 😉):

This is in fact confusing if you have a weak memory like me... I think we might find a way to actually "see" changes on the version-switcher-related stuff on RTD previews, but that is another discussion!

@damianavila
Copy link
Collaborator

Btw, there is another issue with this PR... if you land into stable: https://pydata-sphinx-theme.readthedocs.io/en/stable/ and try to go any other version, it will fail, ie. https://pydata-sphinx-theme.readthedocs.io/en/vv0.7.2/
Notice the vv0.7.2. I think we may need to make a 0.8.1 release when #599 is merged so stable can actually redirect properly to other versions... although 0.8.0 will be always broken 😢? (unless we literally hack something to fix it... although it is late here and I might be missing something).

@choldgraf
Copy link
Collaborator

choldgraf commented Feb 26, 2022

IMO we should fix it, release 0.8.1, and then remove 0.8.0 from existence in the dropdown - in general, I only think we need to display the versions of the latest release on a major version (or, for each minor release for now maybe)

@jorisvandenbossche
Copy link
Member Author

Btw, there is another issue with this PR... Notice the vv0.7.2.

Ah, good catch, I didn't think about that when changing the url_template ..
We can now indeed fix it in 0.8.x, and remove 0.8.0 from the dropdown.

But assume that the dropdown already had existed for a longer time (eg also in 0.6 and 0.7 docs), then this would not have been that easy to "fix". So this points a bit to an inherent limitation of changing things in future versions after an initial set-up? Which makes our current approach not super robust to changing urls?
Although you can probably always fix it by using full urls instead of the url template, thanks to #575 (I suppose this is the generic solution for projects that might change url template at some point, but want to keep working links in older docs?)

@choldgraf
Copy link
Collaborator

That's a good point @jorisvandenbossche - I'm actually wondering if the approach you describe - giving fully-resolved URLs instead of a template - is actually a better solution in general with less complexity. I'll write up an issue to discuss

@choldgraf
Copy link
Collaborator

👉 #600

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.

4 participants