✨ NEW: add version switcher #436
Conversation
drammock
force-pushed
the
redo-switcher
branch
from
78d5eff to
b261510
Compare
drammock
changed the title
|
A comment from the peanut gallery: you can get the versions using Read the Docs API v2 like this |
cool! Though not all projects using this theme are using RTD, so we still need to support a pre-written JSON file. Do you know how to do that API query in javascript (used when |
|
@drammock I have very little knowledge of JavaScript, but I'd do something like this: let versions_api = 'https://readthedocs.org/api/v2/version/?project__slug=' + 'pydata-sphinx-theme'
var headers = new Headers({
'Access-Control-Allow-Origin': '...'
});
fetch(versions_api, {method: 'GET', headers: headers}).then((response) => {
return response.json();
}).then((json) => {
results = json['results'];
});There's CORS considerations (that's what the |
drammock
force-pushed
the
redo-switcher
branch
2 times, most recently
from
8babe19 to
01f471c
Compare
There was a problem hiding this comment.
I think this looks really nice - thanks for this!
My intuition is that this implementation feels much simpler than #433, but I may not be the best person to judge this. It's hard to argue with the difference in "diff size" though...I do feel like the JS in this PR makes more sense to me.
So I guess the question is whether there is any significant difference in functionality between this and #433. Can anybody think of anything?
I wonder if this is something that @bollwyvl would be willing to give a review on? Anything you could do to help us choose one or the other implementation? I would of course also love to hear @ThuWangzw's thoughts on whether this implementation would work for your use-case, if you think there's anything better about one or the other, and why.
| // | ||
| var parts = template.split(/[\{\}]/); | ||
| version_ix = parts.indexOf("version"); | ||
| language_ix = parts.indexOf("language"); |
There was a problem hiding this comment.
ah I see you have language here too....maybe this dropdown support both language and version somehow? Maybe I'm not understanding...
|
It's implemented currently to do either language or version or combine language and version into one switcher. If that's not desirable I think I can imagine how to make separate switchers that play nicely with each other.
My rationale for the current design was that it easily handles the case where not all versions are available in all languages. That is a little trickier with 2 separate switchers but ought to be possible. LMK which you folks prefer.
…-------- Original Message --------
On Aug 25, 2021, 17:49, Chris Holdgraf wrote:
@choldgraf commented on this pull request.
I think this looks really nice - thanks for this!
My intuition is that this implementation feels much simpler than [#433](#433), but I may not be the best person to judge this. It's hard to argue with the difference in "diff size" though...I do feel like the JS in this PR makes more sense to me.
So I guess the question is whether there is any significant difference in functionality between this and [#433](#433). Can anybody think of anything?
I wonder if this is something that ***@***.***(https://github.com/bollwyvl) would be willing to give a review on? Anything you could do to help us choose one or the other implementation?
---------------------------------------------------------------
In [docs/_static/switcher.json](#436 (comment)):
> @@ -0,0 +1,59 @@
+[
could we call this version-switcher.json to dis-ambiguate from a language switcher?
alternatively, could we support multiple switcher configs with a single file?
---------------------------------------------------------------
In [pydata_sphinx_theme/_templates/switcher.html](#436 (comment)):
> + <button type="button" class="btn btn-primary btn-sm navbar-btn dropdown-toggle" id="switcher_button" data-toggle="dropdown">
+ {{ switcher_prefix }}{{ release }}
+ <span class="caret"></span>
+ </button>
+ <div id="switcher" class="dropdown-menu list-group-flush py-0" aria-labelledby="switcher_button">
+ <!-- dropdown will be populated by javascript on page load -->
+ </div>
+</div>
+
+<script type="text/javascript">
+// Function to construct the target URL from the JSON components
+function buildURL(template, entry) {
+ //
+ var parts = template.split(/[\{\}]/);
+ version_ix = parts.indexOf("version");
+ language_ix = parts.indexOf("language");
ah I see you have language here too....maybe this dropdown support both language and version somehow? Maybe I'm not understanding...
—
You are receiving this because you were mentioned.
Reply to this email directly, [view it on GitHub](#436 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAN2AUYV4O5AU6H6OF6TOZTT6VXOPANCNFSM5BESPFTA).
|
|
@drammock it would be helpful if you included documentation for this feature in the PR. I think that this is the easiest way to understand "from a user's perspective" what using the feature would be like. For example, I didn't understand that both language and version would be encoded here. How would that work? How simple would it be? What if somebody had 5 languages and 20 versions? Would they need to have an 80-item list in the JSON file? Understanding that UX is an important question. |
Yes, exactly.
Yes, I mentioned in the PR description that documentation is missing... I was waiting to explain in detail how it works until I was sure that the way that it currently works was going to be acceptable. (Trying to avoid having to redo the docs if interface changes are requested). But maybe at this point I should accept that as inevitable. I'll write some docs as soon as I'm back from holiday (01 Sept) |
|
Hmmm - maybe a simpler solution would be to define "version" as a top-level configuration list, and "language" as a sub-list inside each "version" entry? So instead of: {
"version": "v0.6.1",
"language": "en"
},
{
"version": "v0.6.1",
"language": "fr"
},
{
"version": "v0.5.2",
"language": "en"
},
{
"version": "v0.5.2",
"language": "fr"
},
{
"version": "v0.5.1",
"language": "en"
},you'd have something like: {
"version": "v0.6.1",
"languages": [
"en",
"fr",
],
},
{
"version": "v0.5.2",
"languages": [
"en",
"fr",
],
},
{
"version": "v0.5.1",
"languages": ["en"],
}, |
|
Friendly ping on this one! Is there anything that needs to be discussed here? @jorisvandenbossche I think it would be really helpful to get your thoughts, I get the feeling we are hanging because it's unclear if there is buy-in for one implementation or another from the team |
|
thanks for the ping @choldgraf. I've been working on this locally and got hung up on making it work with 2 separate dropdowns (1 language switcher, 1 version switcher). I've got the version-switcher working now (only shows you versions available in the currently-viewed language) but not the language switcher. I could definitely use some input / reassurance that I'm not struggling to get something to work that ultimately won't be used. Here's how I'm currently doing it locally (not yet pushed to this PR):
{
"version": "2.1",
"languages": ["en", "es"]
},
{
"version": "2.0",
"languages": ["en"]
},
{
"version": "1.0",
"languages": ["en", "zh", "es"]
}Then I think:
Does that sound like the right behavior? Is this all too complicated, and I should just stick to the version switcher for now and kick the can down the road on the language switcher? As an aside, it would be helpful for testing if there were actually a version of the docs published under some language other than english (even if it were just the homepage, and all it said was "this is a placeholder pretending to be a French translation of our docs, for testing purposes"). If someone can either make that file in a separate PR, or tell me where that file should live in order to get properly deployed to https://pydata-sphinx-theme.readthedocs.io/**fr**/*version*/, I'd be grateful. |
|
@astrojuanlu or @humitos do either of you have suggestions for how language/version switching should be done in a UX-friendly manner? How does ReadTheDocs handle this kind of logic? I think we should just try to follow convention if there is one out there. My intuition is that as a first step, we could avoid some of the more complex logic you describe and just do the following:
This will create some tension points when people click a language on a version where there's no corresponding language file, and vice versa. However, IMO those could be future improvements to add more conditional logic, and shouldn't block a working feature. |
|
(slowly getting myself updated on this topic)
There is one thing that this PR does not do (I think), which is to preserve the page that you are on (although I didn't check if #433 is doing that, but so that could be a difference) Especially for larger sites, I think this is a quite important feature of a version switcher (eg in the Arrow docs, we have subdocs for python, c++, java, etc, and you want to stay within those subdocs when switching versions). |
This is a good point. And one have to keep in mind a fallback in case some pages got removed (either trying a search or going to the main index). |
|
This PR already handles that case. I'm on holiday at the moment and responding from a phone, so forgive me for not linking to the exact lines of JavaScript that do this. Please read the code comments to find it (look for "see if the page exists" and "update the target URL"). It defaults to link to the homepage of the other version of the docs, but then checks if the exact page exists and if so changes the link target. |
|
@drammock ah, sorry, I indeed see code that should be doing this. I doesn't work on the readthedocs preview, though, so therefore I assumed this was not yet implemented (or still has a bug). But I assume this might be because on the preview, the url for the other versions has a different base than the PR preview itself? (https://pydata-sphinx-theme--436.org.readthedocs.build/ vs https://pydata-sphinx-theme.readthedocs.io) Will test this out locally. |
Yes that is exactly the issue. The browser console log shows these as blocked CORS requests. I think it's possible to configure the website server to allow requests from the CI server, but I don't have permissions to make such changes |
|
Hi folks, sorry for the late reply! For reference, this is how a multi-lingual, multi-version doc menu looks like:
(From https://docs.python-requests.org/en/latest/) I'm not sure if @choldgraf question above was about the UX or about the implementation. My colleagues @nienn and @humitos have more context on those (and it would be nice to have a tl;dr somewhere since there are lots of details in the conversation here). |
I've seen version switchers implemented in many different ways and I think that sooner or later they have found cases that are not supported, or supporting them, got really complex 😄 . Unfortunately, I don't have a great suggestion to provide here that you can follow as a great example. I would like to mention considering the case for the versions being loaded dynamically (I haven't check the code, but I think the as @jorisvandenbossche mentions
Read the Docs will link you to the same document that you are currently reading when you click on a different language. This is good and bad at the same time 😄 since it doesn't check that document exists in the target language, so you may end in a 404. I suppose that the Another problem that this may have is that the document could exist in the target language, but maybe with a different name because in the newer version it was renamed (consider the case that If you decide to follow this path, the JSON gets simplified because the versions can be just another different list since it will be the same for all the versions. After checking the code (*), I wanted to mention that I worked on projects where they had different hosting for different versions (legacy reasons) and they required using different URLs on the version selector, so they had a
To make this work in both places, (*) I found that the HEAD trick is already implemented 💪🏼 I haven't check the other implementation mentioned in this PR, but this one looks good to me. It took me a few minutes to check the diff and understand the code. Looks clean to me! |
| // Function to populate the (release, language) version switcher | ||
| (function () { | ||
| // get JSON config | ||
| $.getJSON("{{ switcher_json_url }}", function(data, textStatus, jqXHR) { |
There was a problem hiding this comment.
Considering that switcher_json_url = "/_static/switcher.json" and this code will be hosted at /language/version/, shouldn't that JSON url be relative instead?
There was a problem hiding this comment.
If you always want it to point to the same versions.json file (so you only have to update a single file if a new release needs to be added), I think it needs to be a absolute path?
While trying this out, I actually accidentally specified this path in conf.py as a relative path, and then the version dropdown also only works on top-level pages. Because once you are on a page in a subdirectory, the relative path isn't correct.
(so we should at least document it clearly, or could also check for it and raise an informative error, or automatically fix it)
There was a problem hiding this comment.
I think I didn't express myself properly.
I was trying to say that the path should be /en/latest/_static/switcher.json on one side.
On the other side, I thought that each documentation version would have a switcher.json file containing only the versions released until that moment. So, as you already mentioned, it would get outdated once a new version is released. This is what Read the Docs solves by using an API endpoint. Now, I understood what you said regarding using the same JSON file for all the versions --which makes sense to me. However, the path still requires /en/latest/ prefix. At least to work on Read the Docs 😄
There was a problem hiding this comment.
@humitos can you clarify your last sentence from the previous comment:
However, the path still requires
/en/latest/prefix. At least to work on Read the Docs smile
Why does the path still require /en/latest?
Now, for this whole PR to work for a user, they need to be able to put a json file with the list of versions somewhere on the server (at a fixed location). So that is maybe a problem for someone using readthedocs for hosting their docs?
Although I suppose at that point, you could alsojust include the json file in the static files of your docs. And then you can have it point to the latest docs to always use the latest up to date json file (the json files in the older docs will still be present, but just be unused).
Which means that the user can use switcher_json_url = "/en/latest/_static/switcher.json" in their configuration to have it working on readthedocs.
And while writing this / thinking this through, I now realize this is exactly what you said above 😄
(but I thought you were trying to say there is still something missing in the code of the PR to have it work on readthedocs, while if my above reasoning is correct, this is only about properly documenting which path to set in the configuration if you want to use this version switcher on readthedocs (instead of using the one of readthedocs itself).
| }); | ||
| // see if the current page exists in the other versions of the docs | ||
| $.each(data, function(index, entry) { | ||
| let filePath = "{{ pagename }}.html"; |
There was a problem hiding this comment.
I think this will fail if the documentation uses dirhtml builder, since the URLs don't contain .html.
There was a problem hiding this comment.
But for a dirhtml you might also don't care about a working dropdown? (not very familiar with dirhtml though)
There was a problem hiding this comment.
Yeah, I suppose you will still want a working dropdown. Or maybe, I'm not understanding why you wouldn't 😄 . dirhtml works in the same way as html builder; the only difference is that instead of using path/to/page.html it will create path/to/page/index.html and then the URL will be path/to/page/
See https://www.sphinx-doc.org/en/master/usage/builders/index.html#sphinx.builders.dirhtml.DirectoryHTMLBuilder for more info
There was a problem hiding this comment.
OK, I just didn't really know what dirhtml was (I was confusing it with a zipped dir, which you can provide as a download option, and that is typically to use locally, in which case you probably don't care about version dropdowns, unless you download the docs for different versions :))
But indeed, for dirhtml it would be nice to have it working (no idea how often it is used).
| $.ajax({ | ||
| type: 'HEAD', | ||
| url: `${entry.url}${filePath}`, | ||
| // update the target URL if it exists | ||
| success: function() { | ||
| $("#switcher").children()[index].setAttribute("href", `${entry.url}${filePath}`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I think this implementation could be optimized a little. I understand that a HEAD request will be done per version each time a new page is opened, but maybe none of those links will be clicked. Imagine if there is a project with hundred active versions (yes! they exist 😸 )
So, I'd suggest always put the href but intercept the onclick event and do the check on the fly before redirecting.
There was a problem hiding this comment.
Besides, having a fr version shown in the switcher but not being able to click on it and not knowing why will be confusing, IMO.
|
I don't know if this has been explored in other issues or discussions, but there's a number of projects doing their own version switchers. I just found that Blender has two dropdowns: https://docs.blender.org/manual/en/latest/
|
|
Reading through the docs, that's a nice and clear overview! (thanks @drammock and @choldgraf) |
|
I haven't followed up on this for a long time, sorry. I think the current implementation is better than #433. So #433 could be closed when #436 merged or even now. And I suggest adding @ThuWangzw and @xxr3376 as co-authors, they completed the main design and code implementation in MegEngine#1 and #433, some ideas have been absorbed into the current implementation. @ThuWangzw You can also help look at this implementation to see if there are any doubts. |
|
Sorry I didn't follow this pr for such a long time. I think the current implementation is much concise and easy to use than #443! @choldgraf could I close #443 now? |
There was a problem hiding this comment.
Considering that @drammock @ThuWangzw @xxr3376 and @MegChai have already put a lot of work into this problem in general, and since @drammock doesn't have the cycles to promise a quick turnaround on @damianavila's suggestions above. How about we do the following:
- Rebase and make sure the tests are happy
- In our changelog item, attribute @drammock @ThuWangzw @xxr3376 and @MegChai since this was a team effort across multiple PRs with a lot of discussion!
- Leave open for 24 hours in case there are final comments
- Merge this PR (unless there is something relatively minor somebody would like, but I'm +1 as-is)
- Follow up with the implementation improvements that @damianavila suggested in another PR
What do folks think about that?
(and to reiterate, thanks @ThuWangzw @xxr3376 and @MegChai for your collaboration and discussion on this! I think you can close the other PR now if you like. I believe that @damianavila hopes to bring in some of the ideas in that PR into this one, but we should do that in a follow-up rather than in this PR)
The plan makes total sense to me, some comments on each step:
@drammock do you want to take care of the merge conflict? Otherwise, I can do it!
I will take care of that!
Since we are on Thanksgiving time, let's keep this open until early next week, so everyone has time to do a final comment.
IIRC, there was some code change suggestion to be applied before merging, right @drammock?
I will take care of that as well!! |
|
I will maybe also first merge #514 (which moves many pages around), as it might be easier to update this PR (which I can do) as the other way around |
|
@jorisvandenbossche, #514 was the other PR I was interested in reviewing and testing next!!
If you feel that way, and since you know more about the codebase, I will switch gears to look into that PR so we can get it merged ASAP and then push again on this one (which I really want to get it merged as soon as we can 😉 !) |
|
@jorisvandenbossche, #514 will take me a little bit more of time to digest and properly test it. Btw, I have tested this one locally (workarounding CORS as commented here: https://github.com/pydata/pydata-sphinx-theme/pull/436/files#r758652197) as well and it seems to work as advertised/documented 😉 . |
There was a problem hiding this comment.
LGTM after the merge conflicts are fixed and the last changes indicated in https://github.com/pydata/pydata-sphinx-theme/pull/436/files#r741498089 are committed.
|
Woohoo, this is in! :) Thanks a lot to all who participated in this effort! (and I included everyone mentioned above as co-author of the commit) It would now be good if people can test this with their packages to ensure everything is working fine before doing a release. There is also one follow-up issue (apart from #507 for the language switcher), which is to look into moving the js code out of the template (cfr #436 (comment)). @damianavila is this someting you might be able to look into? |
Yes, I will create an issue for that and follow up with a PR to implement that idea. |
|
The issue was opened here: #522 |
This PR is an alternative implementation to #433. It does the following:
_templates/switcher.htmlto create and populate a version-switcher dropdown.docs/_static/switcher.jsonas the source of version infotheme.confwith two new variables (switcher_json_urlandswitcher_template_url)docs/conf.pywith demo-site-appropriate values for those two new variablesOne thing that is missing is a narrative explanation in the docs about how to configure and use this feature.
closes #23
closes #433