add og:title to the docs #1508
add og:title to the docs #1508
Conversation
There was a problem hiding this comment.
Wouldn’t we need an escape as well to protect from titles with double quotes? Considering the og:title value is an attribute while <title> is a tag.
|
Oh, good point. Will check and update. |
docs/templates/docs/doc.html
Outdated
| {% block title %}{{ doc.title|striptags|safe }} | {% trans "Django documentation" %}{% endblock %} | ||
|
|
||
| {% block og_title %}{{ doc.title|striptags|safe }} | {% trans "Django documentation" %}{% endblock %} |
There was a problem hiding this comment.
I think we could wrap with {% with %} to reduce the duplication here
There was a problem hiding this comment.
good idea, though it doesn't seem to work. According to ChatGPT this is because "the scope of the variables in Django's {% with %} statement is limited to the block in which they are defined" (I didn't see this mentioned in the docs but it matches the behavior i'm seeing). So since the variables are used inside separate blocks it doesn't work. We could probably pass a variable in the view if you think it's worth looking into that.
There was a problem hiding this comment.
Also, given that we also now want to escape the one in the meta tag but not the one in the title, I don't think there's any easy way to avoid the duplication.
|
@thibaudcolas I added escaping and confirmed it does the right thing |
There was a problem hiding this comment.
I put together a test for the new behavior (add it to docs/tests.py):
from django.test import RequestFactory
from django.template.loader import render_to_string
class TemplateTestCase(TestCase):
def _assertOGTitleEqual(self, doc, expected):
output = render_to_string(
"docs/doc.html",
{"doc": doc, "lang": "en", "version": "5.0"},
request=RequestFactory().get("/")
)
self.assertInHTML(f'<meta property="og:title" content="{expected}" />', output)
def test_opengraph_title(self):
doc = Document.objects.create(
release=DocumentRelease.objects.create(
lang='en',
release=Release.objects.create(version="5.0"),
),
)
doc.body = "test body" # avoids trying to load the underlying physical file
for title, expected in [
("test title", "test title"),
("test & title", "test & title"),
("test <strong>title</strong>", "test title"),
]:
doc.title = title
with self.subTest(title=title):
self._assertOGTitleEqual(doc, f"{expected} | Django documentation")
docs/templates/docs/doc.html
Outdated
| @@ -2,6 +2,7 @@ | |||
| {% load i18n fundraising_extras docs %} | |||
|
|
|||
| {% block title %}{{ doc.title|striptags|safe }} | {% trans "Django documentation" %}{% endblock %} | |||
| {% block og_title %}{{ doc.title|striptags|escape|safe }} | {% trans "Django documentation" %}{% endblock %} | |||
There was a problem hiding this comment.
I don't think the |safe is needed here, since |escape marks its output as safe as far as I know
There was a problem hiding this comment.
good call: 7545e3c
Also, based on the test even the |escape wasn't necessary (maybe because autoescape is on?). But I left it since it seemed more explicit. But let me know if you think worth removing
There was a problem hiding this comment.
@beingbiplov thank you for the test code! made the suggested changes.
docs/templates/docs/doc.html
Outdated
| @@ -2,6 +2,7 @@ | |||
| {% load i18n fundraising_extras docs %} | |||
|
|
|||
| {% block title %}{{ doc.title|striptags|safe }} | {% trans "Django documentation" %}{% endblock %} | |||
| {% block og_title %}{{ doc.title|striptags|escape|safe }} | {% trans "Django documentation" %}{% endblock %} | |||
There was a problem hiding this comment.
good call: 7545e3c
Also, based on the test even the |escape wasn't necessary (maybe because autoescape is on?). But I left it since it seemed more explicit. But let me know if you think worth removing
Oh good point, I didn't think about I don't seem to have the right permissions to allow the github actions to run on this PR. Can someone from @django/djangoproject-com-maintainters approve it? |
|
@bmispelon good point on tests. I removed the redundant escape |
| for title, expected in [ | ||
| ("test title", "test title"), | ||
| ("test & title", "test & title"), | ||
| ('test "title"', "test "title""), |
There was a problem hiding this comment.
Good thinking on this one too 👍🏻
I don't think so, just a bit of patience, the website team are all volunteers and have other, more paid things to do. |
This uses the same og:title as the page title.
The hooks are already in the base template so this is very straightforward.