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

DOC: fix version switcher #22258

Merged
merged 2 commits into from
Jan 18, 2022
Merged

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Jan 18, 2022

PR Summary

Fix version switcher now that json file is in place.

Note that changes to the json file itself will have to be merged, but the switcher is still not quite working, so lets make sure we understand that first.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@jklymak jklymak added the Documentation: website layout/behavior/styling changes label Jan 18, 2022
@jklymak jklymak marked this pull request as draft January 18, 2022 12:58
@dstansby
Copy link
Member

I followed the link and found the JSON file, but firefox is giving me SyntaxError: JSON.parse: unexpected character at line 105 column 1 of the JSON data - does something need fixing in the JSON file?

@jklymak
Copy link
Member Author

jklymak commented Jan 18, 2022

Maybe, looks like there is a stray comma at the end. However, not sure if that is breaking things....

@jklymak
Copy link
Member Author

jklymak commented Jan 18, 2022

fixed the comma, but of course that won't be in the devdocs version, so we would have to merge that to see if that helps

@jklymak
Copy link
Member Author

jklymak commented Jan 18, 2022

90% sure this will work now - apparently you have to specify the version field. However, they seem to do some fancy stuff to compare the desired page to the current page, so the full list of versions doesnt' seem to get populated? Perhaps will if it is put on devdocs, but I can't tell. Maybe @choldgraf would have a minute to stop by and tell us if we are doing it right.

@choldgraf
Copy link
Contributor

@drammock is probably in a better place to provide input than me, since he built the feature! 😅

@jklymak jklymak marked this pull request as ready for review January 18, 2022 15:31
@jklymak
Copy link
Member Author

jklymak commented Jan 18, 2022

The button now has some version info. I'm hoping that when it is pushed to devdocs it will work. However, I'm not sure - there seems to be a fair bit of JS logic checking for page consistency that I'm not sure applies to our docs.

@drammock
Copy link

apparently you have to specify the version field.

Yes. name field is optional, and will default to being a copy of version if name isn't defined.

However, they seem to do some fancy stuff to compare the desired page to the current page, so the full list of versions doesnt' seem to get populated?

The comparison of desired page to current page does this: if you're viewing "contributing guide" and use the version switcher to go to version 2.2, it looks for the contributing guide within the v-2.2 docs folder subtree. If the page isn't there, it takes you to the 2.2 homepage instead.

The JSON and conf.py both look correct to me after 069a068. I recommend splitting this PR to make the corrections to the JSON go live first, so that the JSON parse error won't stop you from evaluating the effect of the config changes.

One thing to watch out for is that there can be cross-domain resource restrictions (CORS) that prevent pages in the circleCI build (at https://66366-1385122-gh.circle-artifacts.com/* or similar) from accessing JSON assets from another domain (i.e., https://matplotlib.org/). This is definitely happening for you (see lower arrow in screenshot below). I don't pretend to understand CORS very well so I can't offer much advice other than to say that (1) once the requests are no longer cross-domain (i.e., requesting page and requested JSON are both on matplotlib.org) everything should work, and (2) it's possible to configure the host server to allow cross-domain AJAX requests but I've never done it... my impression is that it's considered a bad idea if you're talking about a JSON API for a customer database or something, but probably no big deal if the only JSON on your domain is this version switcher config. I've also read that on recent versions of firefox you can also set about:config setting content > cors > disable to true, though in my brief testing I don't think that gets around the CircleCI -> matplotlib.org issue.

Another strange thing I see in the browser console is a request for a versions.html at the site root. Not sure what that's about? It's the upper arrow in the screenshot.

Screenshot_2022-01-18_11-12-30

One other random thing I happened to notice is that the 2.2 entry goes to version 2.2.4, but what is displayed is actually the 2.2.5 docs. Not likely to cause any problems for users though.

@tacaswell
Copy link
Member

Another strange thing I see in the browser console is a request for a versions.html

This a long standing thing we do on the top-level index for the built docs that links to the latest version. It is a way that we can inject changes into the old dcos without re-rendering them.

@jklymak
Copy link
Member Author

jklymak commented Jan 18, 2022

Thanks @drammock That helps. Hopefully the domain crossing stuff works - I think our devdocs and main site are basically the same (github pages). Understood about the existing pages check.... We actually have a bunch fo redirects in our current docs to pages that have moved, but of course the old docs don't have redirects to new pages.

@drammock
Copy link

We actually have a bunch fo redirects in our current docs to pages that have moved, but of course the old docs don't have redirects to new pages.

yeah, we have the same issue over at MNE-Python. Users viewing old docs and using the switcher to get to current stable/dev will benefit (because the redirect page exists, the switcher will point to that), but using the switcher to go from stable/dev to older doc versions will soft-fail (go to target version's homepage) from pages that have moved.

@jklymak
Copy link
Member Author

jklymak commented Jan 18, 2022

See #22261 for just changing the json

@jklymak jklymak marked this pull request as ready for review January 18, 2022 22:16
@timhoffm timhoffm added this to the v3.6.0 milestone Jan 18, 2022
@timhoffm timhoffm merged commit 62978a2 into matplotlib:main Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: website layout/behavior/styling changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants