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 issue where twitter cards not registering #29

Merged
merged 7 commits into from
Nov 17, 2022
Merged

Conversation

josh146
Copy link
Member

@josh146 josh146 commented Nov 13, 2022

Context: The .. meta:: directive in ReST is supposed to automatically create HTML <meta> tags, and we use this in the QML repo to automatically register Open Graph social media cards. However, this was not working.

image

Description of the Change: Add Jinja2 templating to ensure the OpenGraph card is correctly created.

Benefits: Fixes bug, card will now display on Twitter.

Possible Drawbacks: n/a

Related GitHub Issues: n/a

Copy link

@larawatson larawatson left a comment

Choose a reason for hiding this comment

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

Thanks, Josh!

Copy link
Collaborator

@Mandrenkov Mandrenkov left a comment

Choose a reason for hiding this comment

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

Looks OK although I'm not sure I fully understand the {% if ... %} block. 🤔

.github/CHANGELOG.md Outdated Show resolved Hide resolved
xanadu_sphinx_theme/layout.html Show resolved Hide resolved
Comment on lines 11 to 14
{% if metatags is defined %}
{% set description = metatags.split('\n') %}
{{ description[0] | replace("og:", "") | replace("property", "name") }}
{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code is probably correct, but can you explain what exactly it does? In particular:

  1. Isn't metatags always defined if we have those <meta> tags above?
  2. Why are we printing only the first meta tag? What about the rest?
  3. What does printing actually accomplish here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is very old code that got lost in the great XST merger, so I'm trying to cast my mind back!

Isn't metatags always defined if we have those tags above?

I think this refers to the .. meta:: ReST role. If a ReST page defines a metadata block this way, then this if statement is evaluated as true.

Why are we printing only the first meta tag? What about the rest?

I might be wrong, but my understanding is that there is an assumption that there is only one meta block on the page (see https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#html-meta or https://docutils.sourceforge.io/docs/ref/rst/directives.html#metadata).

So I think description[0] is hardcoding this assumption? But I'm not sure.

What does printing actually accomplish here?

Good question 🤔

It looks like it is converting the following,

.. meta::
    :property="og:description": An example demo using the Xanadu Sphinx Theme generated using Sphinx Gallery.
    :property="og:image": https://pennylane.ai/qml/_static/wigner.png

into

<meta content="An example demo using the Xanadu Sphinx Theme generated using Sphinx Gallery." property="og:description" />
<meta content="https://pennylane.ai/qml/_static/wigner.png" property="og:image" />

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this refers to the .. meta:: ReST role. If a ReST page defines a metadata block this way, then this if statement is evaluated as true.

Makes sense!

I might be wrong, but my understanding is that there is an assumption that there is only one meta block on the page (see https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#html-meta or https://docutils.sourceforge.io/docs/ref/rst/directives.html#metadata).

So I think description[0] is hardcoding this assumption? But I'm not sure.

I'm not sure that assumption is actually enforced anywhere but I think I can live it. That said, based on the documentation, you would think that

metatags.split('\n')[0]

would simply refer to the first defined <meta> tag. 🤔

Good question 🤔

It looks like it is converting the following,

.. meta::
    :property="og:description": An example demo using the Xanadu Sphinx Theme generated using Sphinx Gallery.
    :property="og:image": https://pennylane.ai/qml/_static/wigner.png

into

<meta content="An example demo using the Xanadu Sphinx Theme generated using Sphinx Gallery." property="og:description" />
<meta content="https://pennylane.ai/qml/_static/wigner.png" property="og:image" />

Ah, that makes sense, but this brings to mind another question:

{{ description[0] | replace("og:", "") | replace("property", "name") }}

Above, we see a call to replace("og:", ""); however, there are clearly occurrences of og: in the output. So what is that filter supposed to be doing?

Copy link
Member Author

Choose a reason for hiding this comment

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

So what is that filter supposed to be doing?

I have no idea 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

So what does the output look like if we remove that filter? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

<meta content="An example demo using the Xanadu Sphinx Theme generated using Sphinx Gallery." property="og:description" />
<meta content="https://pennylane.ai/qml/_static/wigner.png" property="og:image" />

Seems to have made no difference like you thought 😆

xanadu_sphinx_theme/layout.html Show resolved Hide resolved
Co-authored-by: Mikhail Andrenkov <Mandrenkov@users.noreply.github.com>
Copy link
Collaborator

@Mandrenkov Mandrenkov left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me! 🚢

josh146 and others added 3 commits November 14, 2022 22:24
@josh146 josh146 merged commit 8efda02 into master Nov 17, 2022
@josh146 josh146 deleted the fix-twitter-card branch November 17, 2022 02:50
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