Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
✨ NEW: add version switcher #436
✨ NEW: add version switcher #436
Changes from 25 commits
84d0562
5f4a023
93b0aa0
e913a48
cefafeb
0f12757
0ff914e
852cc4e
ac2a57b
26e5988
0328868
2a38901
768a76e
5318996
ab261a3
def2d67
d968eef
fec276c
24da28e
407159d
f009b9d
87c0dc9
191a847
e683a50
3258028
9ad61e2
a010d24
865ed64
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the "last change before merge" suggestion (because it will break the switcher functionality on the CI PR build)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump? I think I've adressed all feedback here, and all that remains is for someone with merge rights to sign off on the new section in the rendered docs, and if they're happy, apply this suggestion and then merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminder to apply this suggestion before merging; it will break the switcher in the PR doc build but is needed for the switcher to work on the real docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understood it right or if I am missing anything in this lengthy context. Can someone please help me with the latest repo with both versions and language drop-down implementation for the RTD theme in Sphinx? @drammock , @astrojuanlu, or @humitos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sachin-Suresh I'm not sure to understand what is your question. I'd recommend you to open a new issue with a longer and detailed description. Otherwise, it'd be hard to parse your question in a PR that has ~150 comments already and 1 year old.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should somehow address this one before merging the PR, @drammock.
We should try to move most of the JS to the
src/js/index.js
file. In general, I believe we should minimize the amount of JS code in the template as far as we can. And we can borrow some ideas from #433 to execute that idea. You can see they are only adding JS code in the template to "persist" the information aboutpagename
andtheme_swticher
. They are doing that using the window object. We may use the same approach or maybe some other ones such asdata-*
attributes that could be more specific? ones (instead of polluting the window namespace) and it might play better with the template...But the important piece, whatever implementation you think about, is the idea/aim to minimize the JS code in the template and group that code in the "natural" JS place we currently have, IMHO.
@drammock, I know this is a long-lasting PR but would you be willing to make this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like many (most?) contributors here, I am not a JS developer, I am a python developer who knows just enough JS to get the job done, and figure out a lot as I go along. I invite/encourage you to push commits to this branch if you have a clear idea of how to do what you suggest. Otherwise I can say that I will intend to do as you suggest, but offer no promises as to the timeline because I have no way of estimating how long it will take me to figure it out / get it right, which means I'm less likely to try to squeeze it in when I have just a few minutes to spare.
If that's not acceptable, another option is to merge now (once merge conflict is fixed, I see this PR has again gone stale) and migrate the JS in a follow-up PR, either by me (when I get around to it) or by anyone else who is inspired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I think it would indeed be good to have our JS in
src/js/index.js
, but if that makes this quite a bit more complicated, I also think that practicality beats purity in this case.Now, from the explanation above, it seems there is a possible solution already. But at the same time, this is in the end also only an implementation detail, not impacting in any way how the end user uses/configures this. So I would say that this therefore is also perfectly well suited as a follow-up (given how long this already has taken).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback @drammock and @jorisvandenbossche!
A follow-up PR seems sensible to me (btw, I had to ask you first @drammock, in case you wanted to do it 😉 ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested (via
console.log()
) to make sure thattryURL
is indeed trying the correct URL, and in my local testing usingnox -s docs-live
, this all works (my browser console shows a200
response to the head request, but then the call is flagged as failing anyway because of the CORS policy on the readthedocs server). The same happens with the PR test build (https://pydata-sphinx-theme--436.org.readthedocs.build/en/436/demo/index.html) because it's still a cross-domain AJAX request. I'm fairly sure that, once merged, these calls will succeed and the version switching will go to the corresponding page in the other docs version (if it exists), rather than the docs homepage for that version. However, it would still be better (if possible) if somebody who has the relevant permissions could enable cross-domain CORS requests at least forhttps://pydata-sphinx-theme--*.org.readthedocs.build
(this won't make it work for testing withlocalhost
, but it should make it work for PR test builds).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am out of my depth here, not being very familiar with CORS and those topics. But I am not sure there is something we can enable ourselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes 2 of us. Personally I would say this: the version switcher still works (you can still switch between docs versions) even in the face of these cross-domain requests getting blocked. In theory, once this PR is merged and live, the requests won't be cross-domain anymore and the behavior should improve (links go to corresponding pages in other doc versions), but even if the behavior doesn't improve, at least we won't be in a broken state (the switcher will still go to the homepage of the correct docs version). So if a conversation with the RTD folks about CORS configuration is too bothersome (or in case they're unwilling to change server config in this way), let's just merge as-is for now and let the live doc build (not the PR build) be our test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from me on giving this a merge and seeing what happens :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to play and test this PR locally with CORS disabled (btw, I do not recommend disabling sec features in the browser unless you know what you are doing) and I can confirm this seems to be working as expected: it redirects to the proper path in the previous (whatever you choose) version of the docs.
At the time to dev/test this sort of issue, there might be other "solutions" using some proxies: https://github.com/Rob--W/cors-anywhere, but it is probably too much stuff for the thing we need to test here (this is why I went wild with an insecure browser test in a controlled environment 😉 ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@choldgraf I'm just calling attention to this line as it's a new idea that occurred to me, not something anyone requested. In the switcher dropdown menu, it highlights the entry for the currently-being-viewed version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it :-)