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

docs: add missing og:video meta tag #69644

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

Marukome0743
Copy link
Contributor

@Marukome0743 Marukome0743 commented Sep 4, 2024

Summary

Add og:video meta tag to <head> output.

Description

Video example was added to the openGraph object in metadata at #66613.
However this commit forgot output result.

Improving Documentation

x-ref: #66613

@ijjk ijjk added the Documentation Related to Next.js' official documentation. label Sep 4, 2024
@ijjk
Copy link
Member

ijjk commented Sep 4, 2024

Allow CI Workflow Run

  • approve CI run for commit: 579e772

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@huozhi huozhi added the CI approved Approve running CI for fork label Sep 10, 2024
@huozhi huozhi enabled auto-merge (squash) September 10, 2024 10:04
@@ -521,6 +521,9 @@ export const metadata = {
<meta property="og:image:width" content="1800" />
<meta property="og:image:height" content="1600" />
<meta property="og:image:alt" content="My custom alt" />
<meta property="og:video:url" content="https://nextjs.org/video.mp4">
Copy link
Member

Choose a reason for hiding this comment

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

lint is failing

Suggested change
<meta property="og:video:url" content="https://nextjs.org/video.mp4">
<meta property="og:video:url" content="https://nextjs.org/video.mp4" />

Copy link
Contributor Author

@Marukome0743 Marukome0743 Sep 10, 2024

Choose a reason for hiding this comment

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

Oh... Thank you for check lint.

BTW, this code is html, not JSX.
That's why we might use plane html tag.

The real output is
<meta property="og:video:url" content="https://nextjs.org/video.mp4">

However, it's not big matter on this section.
Never mind.

Copy link
Member

Choose a reason for hiding this comment

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

yea we're linting them together now, will look for improvement on that later, thanks!

@huozhi huozhi merged commit 25eda2e into vercel:canary Sep 10, 2024
38 checks passed
@@ -521,6 +521,9 @@ export const metadata = {
<meta property="og:image:width" content="1800" />
<meta property="og:image:height" content="1600" />
<meta property="og:image:alt" content="My custom alt" />
<meta property="og:video:url" content="https://nextjs.org/video.mp4" />
Copy link
Contributor

Choose a reason for hiding this comment

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

og:video:url is not a valid property; https://ogp.me/#structured

Unlike og:image, which does have a :url variant, video does not

Copy link
Member

Choose a reason for hiding this comment

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

Good catch, it should be og:video

Copy link
Contributor

@Netail Netail Sep 11, 2024

Choose a reason for hiding this comment

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

The same happens with audio, where it becomes og:audio:url

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice!

In addition, og:image examples still use og:image:url in docs.
We will update the below code and docs.

function getMetaKey(prefix: string, key: string) {
// Use `twitter:image` and `og:image` instead of `twitter:image:url` and `og:image:url`
// to be more compatible as it's a more common format
if ((prefix === 'og:image' || prefix === 'twitter:image') && key === 'url') {
return prefix
}

Copy link
Contributor

Choose a reason for hiding this comment

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

#69959, ahh already did :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI approved Approve running CI for fork Documentation Related to Next.js' official documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants