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 first_image not resolving correctly in subdirectories #61

Merged
merged 8 commits into from
Mar 8, 2022

Conversation

ItayZiv
Copy link
Collaborator

@ItayZiv ItayZiv commented Mar 3, 2022

Fixes #59.
I'm actually not 100% if there is a more proper way to do this is, so the solution is a bit hacky, but this will be dealt with in #60 in a more proper way anyways.
(Another solution would be to use Path(page_url).parent instead of ogp_site_url which would resolve the relative link by itself, but that didn't seem like a cleaner solution, and once again this is a somewhat temporary fix)

@ItayZiv ItayZiv requested a review from TheTripleV March 3, 2022 00:03
@ItayZiv
Copy link
Collaborator Author

ItayZiv commented Mar 3, 2022

I actually ended up swapping it over to use page_url directly, I forgot that because of how urljoin works we don't need to get the parent directory so it ends up looking much cleaner this way.
Also checked the speed and its negligible difference in speed (between .split .rpartition and this), it's a bit slower but almost within margin of error.

@rabernat
Copy link

rabernat commented Mar 3, 2022

FYI I tried this branch locally and confirmed that it solves the problem described in #59. I have actually (temporarily) installed this branch in our RTD env and now the live site is working: https://pangeo-forge.readthedocs.io/en/latest/introduction_tutorial/intro_tutorial_part1.html

Thanks for your extremely quick help on this!

@TheTripleV
Copy link
Member

Could you add in a test case that fails before and passes with this PR?

@ItayZiv
Copy link
Collaborator Author

ItayZiv commented Mar 3, 2022

Apparently a test for this exists (test-image-rel-paths), except its conf.py had the url set to http://example.org/" which doesn't encounter this problem.
On the other hand, this PR will break relative paths as defined in conf.py, which is also untested but while it is mentioned in the readme its barley supported since we don't (yet) copy the images, so they have to be placed in _static or manually added to html_static_path or html_exclude_path.

I'll fix the test and I think ill change all URLs to use something like /en/latest/ since its a very common use case, and I can't imagine something which works with it, not working without.
But we should decide whether to axe support for relative paths until #60 for conf.py on top of field lists (meaning it will only be useful for first_image or fix support (either convert relative paths the old way for ogp_image)

@TheTripleV
Copy link
Member

TheTripleV commented Mar 3, 2022

We shouldn't break existing functionality.
Can you special case the urljoin so that if ogp_image is specified, it will still be relative to the config["ogp_site_url"]?
This way, both existing functionality (ogp_image) and intended functionality (first image) will hold.

But we should decide whether to axe support for relative paths

This PR doesn't need to add relative path support for field lists.

@ItayZiv
Copy link
Collaborator Author

ItayZiv commented Mar 5, 2022

I fixed it, the fix is a bit weird but I think it is ok until #60.

This PR doesn't need to add relative path support for field lists.

I meant maybe we should consider not supporting them for now at all, but never mind that.

@TheTripleV TheTripleV changed the title Fix for first_image not resolving correctly in subdirectories Fix first_image not resolving correctly in subdirectories Mar 8, 2022
@TheTripleV TheTripleV merged commit ada5be9 into wpilibsuite:main Mar 8, 2022
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.

first_image URLs not resolving correctly on RTD
4 participants