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

Add full support for image, video and audio tags. #60

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ItayZiv
Copy link
Collaborator

@ItayZiv ItayZiv commented Mar 2, 2022

Closes #53.
Adds full support for image, video, and audio og tags:

  • Relative path support for images
  • Relative path support for video and audio
  • Automatic mime type detection and validation
  • Width/Height tag support for images
  • Width/Height tag support for video

Currently, whether an image is configured in conf.py or in field lists, if it starts with / it is treated as relative to the root, if it doesn't it's treated relative to the current document. I think this functionality is fine and allows you to set up different images for each file without field lists if you use the same naming scheme (i.e. `images/docname/opengraph.png). but open to comments.

return urljoin(url, image_path)


def note_image(image_path: str, docname: str, app: Sphinx):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Open for comments on any of the matters mentioned in the comments in this function

Comment on lines 154 to +170
if "og:image" in fields:
image_url = fields["og:image"]
ogp_use_first_image = False
ogp_image_alt = fields.get("og:image:alt")
fields.pop("og:image", None)
image_url = image_abs_url(
fields.pop("og:image"), config["ogp_site_url"], docname, app
)
image_alt = fields.pop("og:image:alt", None)
elif fields.get("ogp_use_first_image", config["ogp_use_first_image"]) and (
first_image := doctree.next_node(nodes.image)
):
# Use page url, since the image uri at this point is relative to the output page
image_url = image_abs_url(first_image["uri"], page_url)
image_alt = first_image.get("alt", fields.pop("og:image:alt", None))
else:
image_url = config["ogp_image"]
ogp_use_first_image = config["ogp_use_first_image"]
ogp_image_alt = fields.get("og:image:alt", config["ogp_image_alt"])

fields.pop("og:image:alt", None)

if ogp_use_first_image:
first_image = doctree.next_node(nodes.image)
if (
first_image
and Path(first_image.get("uri", "")).suffix[1:].lower() in IMAGE_MIME_TYPES
):
image_url = first_image["uri"]
ogp_image_alt = first_image.get("alt", None)
#
image_url = image_abs_url(
config["ogp_image"], config["ogp_site_url"], docname, app
)
image_alt = config["ogp_image_alt"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Open for comments on this implementation I have a few ideas but still would like to hear anything else, currently uses walrus operator (would like to avoid that if possible, since currently, we support down to 3.6)

image_url = first_image["uri"]
ogp_image_alt = first_image.get("alt", None)
#
image_url = image_abs_url(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Optionally, we could rework this so that if a relative path is specified in ogp_image (i.e, path doesn't start with a slash), then we will assume that every document contains an image to be used for the metadata of that document. (So if ogp_image would be ../images/og.png, a site could have a easier time setting a separate image for each document)

@Daltz333
Copy link
Member

Daltz333 commented Jan 8, 2023

Is this PR stale?

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.

Full support for image, video and audio tags.
2 participants