search

wpilibsuite / sphinxext-opengraph

Sphinx extension to generate unique OpenGraph metadata
https://sphinxext-opengraph.readthedocs.io
Other
74 stars 27 forks source link

Fix first_image not resolving correctly in subdirectories #61

Closed ItayZiv closed 2 years ago

ItayZiv commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

ItayZiv commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.