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 canonical URLs with dirhtml builder #870

Conversation

justinludwig
Copy link

As suggested by @ericholscher in #285, using the logic from the
readthedocs-sphinx-ext package.

Currently if you set the canonical_url theme option (for example, to
https://example.com/), a canonical link like the following will be
generated for source files like foo/bar.rst:

<link rel="canonical" href="https://example.com/foo/bar.html"/>

This is good for the html builder, but doesn't match the output of the
dirhtml builder. This commit changes canonical link generation like
the above example to the following when the dirhtml builder is used:

<link rel="canonical" href="https://example.com/foo/bar/"/>

Canonical link generation remains the same as before when the dirhtml
builder is not used.

As suggested by @ericholscher in readthedocs#285, using the logic from the
readthedocs-sphinx-ext package.

Currently if you set the `canonical_url` theme option (for example, to
`https://example.com/`), a canonical link like the following will be
generated for source files like `foo/bar.rst`:

```
<link rel="canonical" href="https://example.com/foo/bar.html"/>
```

This is good for the `html` builder, but doesn't match the output of the
`dirhtml` builder. This commit changes canonical link generation like
the above example to the following when the `dirhtml` builder is used:

```
<link rel="canonical" href="https://example.com/foo/bar/"/>
```

Canonical link generation remains the same as before when the `dirhtml`
builder is not used.
@Blendify Blendify self-requested a review February 23, 2020 17:20
Copy link
Member

@Blendify Blendify left a comment

Choose a reason for hiding this comment

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

I would like to see this change added to sphinx itself first: https://github.com/sphinx-doc/sphinx/blob/master/sphinx/themes/basic/layout.html

This way we can inherit the html from sphinx.

@stsewd
Copy link
Member

stsewd commented Oct 7, 2020

Thanks for the contribution, this is in sphinx itself now https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html_baseurl, we should deprecate the canonical_url option in favor of html_baseurl https://github.com/sphinx-doc/sphinx/blob/e886cc21710e23bc53af83d011131828b1020848/sphinx/themes/basic/layout.html#L135-L137

@stsewd stsewd closed this Oct 7, 2020
@stsewd
Copy link
Member

stsewd commented Oct 7, 2020

#1003

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.

None yet

3 participants