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: resolve path of switcher when using relative #1344

Merged
merged 12 commits into from
Jun 14, 2023

Conversation

12rambau
Copy link
Collaborator

@12rambau 12rambau commented Jun 8, 2023

Fix #647

check if the json file is relative, if it's the case I'm creating a dynamic absolute path from the root of the current website, making the documentation work in dev mode.

@12rambau 12rambau marked this pull request as ready for review June 8, 2023 06:42
@12rambau 12rambau marked this pull request as draft June 8, 2023 15:32
@drammock
Copy link
Collaborator

drammock commented Jun 10, 2023

@12rambau I pushed a commit that makes this work for me locally. You can test it by setting

json_url = "_static/switcher.json"

in conf.py. LMK what you think about this approach / feel free to revert if you have a better approach in mind.

@12rambau
Copy link
Collaborator Author

thanks for the help, but it's not going to work, the redirection trick I used need to be applied to fetch the json file, not to read the url (which is usually correctly formed in the json itself). I'll try to reshape your solution a bit.

@12rambau
Copy link
Collaborator Author

@drammock I updated your code and it's now working for me both locally and the PR build. What do you think ?

@12rambau 12rambau marked this pull request as ready for review June 12, 2023 15:47
Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is almost there. Switcher is now populated and functional on local builds with a relative path to JSON, and on RTD CI build with an absolute path. Wondering if we should add a test that uses relative-path JSON, and make sure things get populated correctly? (since we're normally not using relative path in our own docs, it seems too easy to introduce a regression).

There is still a problem with the "link to corresponding page" vs "link to homepage" functionality. (Example is that from the user guide page on CI build clicking latest gets you to homepage of latest, not user-guide page of latest. This is probably because .cloneNode wipes out onclick attributes, so the onclick needs to be added after cloning. I can push a commit testing that shortly.

@12rambau
Copy link
Collaborator Author

since we're normally not using relative path in our own docs, it seems too easy to introduce a regression

I'm not sure a test is necessary here as we use a relative link for every PR/latest. as long as we at least try to display the button we will see the cullprit.

@drammock
Copy link
Collaborator

as we use a relative link for every PR/latest

really? Our docs/conf.py has an absolute URL in it, won't that get used on PR builds (and latest)?

@12rambau
Copy link
Collaborator Author

12rambau commented Jun 14, 2023

yep the json url is switched if you are in a PR build with a "dev" version to a relative path:

json_url = "_static/switcher.json"

that's why I was working on it initially, to make it work in PRs.

@drammock
Copy link
Collaborator

yep the json url is switched if you are in a PR build with a "dev" version to a relative path

Ha OK sorry I didn't realize that before. So no new test needed.

There is still a problem with the "link to corresponding page" vs "link to homepage" functionality.

I didn't manage to fix this by just putting onclick after the cloning, but based on the console errors I think it's a CORS problem which means it might actually work correctly on the live site. So I'm +1 for merge on this one, and we can chase the "corresponding page" issue afterward if necessary, maybe as part of addressing #1343

@12rambau 12rambau merged commit b36c6d8 into pydata:main Jun 14, 2023
16 checks passed
@12rambau 12rambau deleted the switcher branch June 14, 2023 16:35
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.

Version dropdown menu in local development mode
2 participants