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 head metadata support for AMP Stories via amp_story_head action #13446

Closed
wants to merge 3 commits into from

Conversation

westonruter
Copy link
Contributor

Summary

In ampproject/amp-wp#3039 a new amp_story_head action was added for plugins to output additional metadata in AMP Stories (ampproject/amp-wp#2968). This hook is analogous to wp_head and embed_head in core. Support for AMP Stories was submitted for Jetpack's opengraph logic in Automattic/jetpack#13416. This PR does the same for Yoast. (Additional core metadata is being added to Stories in ampproject/amp-wp#3175.)

This PR can be summarized in the following changelog entry:

  • Add support for adding metadata to AMP Stories (the amp_story post type) from the official AMP plugin.

Relevant technical choices:

  • In the same way that wp_head is hooked/unhooked, the same is done for the amp_story_head action.

Test instructions

This PR can be tested by following these steps:

  1. Generate a build of the AMP plugin from the develop branch.
  2. Activate the Stories experience in the AMP settings screen.
  3. Publish a story.
  4. The head on the frontend story should have its output modified as follows with this change:
6a7,18
> 	<meta property="og:locale" content="en_US">
> 	<meta property="og:type" content="article">
> 	<meta property="og:title" content="Bison2 - WordPressDevs">
> 	<meta property="og:url" content="https://wordpressdev.lndo.site/stories/bison-2/">
> 	<meta property="og:site_name" content="WordPressDevs">
> 	<meta property="og:image" content="https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-4-1024x668.jpg">
> 	<meta property="og:image:secure_url" content="https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-4-1024x668.jpg">
> 	<meta property="og:image:width" content="1024">
> 	<meta property="og:image:height" content="668">
> 	<meta name="twitter:card" content="summary_large_image">
> 	<meta name="twitter:title" content="Bison2 - WordPressDevs">
> 	<meta name="twitter:image" content="https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-4.jpg">
86a99,140
> 	<!-- This site is optimized with the Yoast SEO plugin v12.0 - https://yoast.com/wordpress/plugins/seo/ -->
> 	<!-- Admin only notice: this page does not show a meta description because it does not have one, either write it for this page specifically or go into the [SEO - Search Appearance] menu and set up a template. -->
> 	<link rel="canonical" href="https://wordpressdev.lndo.site/stories/bison-2/">
> 	<script type="application/ld+json" class="yoast-schema-graph yoast-schema-graph--main">{
> 		"@context": "https://schema.org",
> 		"@graph": [
> 			{
> 				"@type": "WebSite",
> 				"@id": "https://wordpressdev.lndo.site/#website",
> 				"url": "https://wordpressdev.lndo.site/",
> 				"name": "WordPressDevs",
> 				"potentialAction": {
> 					"@type": "SearchAction",
> 					"target": "https://wordpressdev.lndo.site/?s={search_term_string}",
> 					"query-input": "required name=search_term_string"
> 				}
> 			},
> 			{
> 				"@type": "ImageObject",
> 				"@id": "https://wordpressdev.lndo.site/stories/bison-2/#primaryimage",
> 				"url": "https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-4.jpg",
> 				"width": 2700,
> 				"height": 1761,
> 				"caption": "Bizon!!"
> 			},
> 			{
> 				"@type": "WebPage",
> 				"@id": "https://wordpressdev.lndo.site/stories/bison-2/#webpage",
> 				"url": "https://wordpressdev.lndo.site/stories/bison-2/",
> 				"inLanguage": "en-US",
> 				"name": "Bison2 - WordPressDevs",
> 				"isPartOf": {
> 					"@id": "https://wordpressdev.lndo.site/#website"
> 				},
> 				"primaryImageOfPage": {
> 					"@id": "https://wordpressdev.lndo.site/stories/bison-2/#primaryimage"
> 				},
> 				"datePublished": "2019-08-28T00:00:11+00:00",
> 				"dateModified": "2019-09-03T18:53:24+00:00"
> 			}
> 		]
> 	}</script><!-- / Yoast SEO plugin. -->
92,93d145
< 	<link rel="prev" title="Test" href="https://wordpressdev.lndo.site/stories/test-5/">
< 	<link rel="canonical" href="https://wordpressdev.lndo.site/stories/bison-2/">
100,122d151
< 	<script type="application/ld+json">{
< 		"@context": "http:\/\/schema.org",
< 		"publisher": {
< 			"@type": "Organization",
< 			"name": "WordPressDevs",
< 			"logo": "https:\/\/wordpressdev.lndo.site\/content\/uploads\/2019\/02\/cropped-Security-Photo-4.png"
< 		},
< 		"@type": "BlogPosting",
< 		"mainEntityOfPage": "https:\/\/wordpressdev.lndo.site\/stories\/bison-2\/",
< 		"headline": "Bison2",
< 		"datePublished": "2019-08-28T00:00:11+00:00",
< 		"dateModified": "2019-09-03T18:53:24+00:00",
< 		"author": {
< 			"@type": "Person",
< 			"name": "ˈwɛsˌtn̩ \\o\/ ˈɹuːˌɾɚ (Рутер)"
< 		},
< 		"image": [
< 			"https:\/\/wordpressdev.lndo.site\/content\/uploads\/2019\/08\/American_bison_k5680-1-4-696x928.jpg",
< 			"https:\/\/wordpressdev.lndo.site\/content\/uploads\/2019\/08\/American_bison_k5680-1-4-928x928.jpg",
< 			"https:\/\/wordpressdev.lndo.site\/content\/uploads\/2019\/08\/American_bison_k5680-1-4-928x696.jpg",
< 			"https:\/\/wordpressdev.lndo.site\/content\/uploads\/2019\/08\/American_bison_k5680-1-4.jpg"
< 		]
< 	}</script>

Outstanding Issues

⚠️ The image metadata being generated by Yoast is not including the sizes that Google Search is looking for. See ampproject/amp-wp#2930 for background on this. In particular, the image should be an array containing:

  1. The portrait image size.
  2. The square image size.
  3. The landscape image size.
  4. The normal featured image.

Google Search would then pick the right image aspect ratio based on the needs of the given context. This PR needs to be amended to also ensure that these image sizes are included.

⚠️ Additionally, Yoast does not seem to be including the publisher logo. Search needs there to be logo provided, even falling back to the WordPress logo. Again, refer to ampproject/amp-wp#2930

👉 Please feel free to amend the PR with the required changes!

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended

@jdevalk
Copy link
Contributor

jdevalk commented Sep 4, 2019

Hey @westonruter thanks for your pull :)

Couple of questions:

  • Can you have a look in your settings of Yoast SEO and tell me what's under SEO -> Search Appearance -> General (the first tab) -> Knowledge Graph & Schema.org ? It's weird to see no Person or Organization output in your example output as our code basically always has at least one of those.

  • The image attribute in the pull you refer to seems to throw an array of images into a single image attribute. If I look at the Schema definition of BlogPosting, Article or basically any Thing, image can either be a full imageObject or just a URL. Putting in an array of URLs seems to me as to be stretching the specification.

    Wouldn't it be better if we change that to be 4 image attributes with 4 URLs or, even better, fully expanded imageObject's? Then the size of the images would be in the actual metadata, seems much more sensible and reusable web-wide to me, but I'm not sure whether that'll be parsed correctly on Google's end?

  • Your default type is BlogPosting apparently in AMP Stories, with your pull it becomes a WebPage when Yoast SEO is on, I'm not sure that'll work the same way, should we just keep it BlogPosting? We could also choose to make it an Article, simply by using the wpseo_schema_article_post_type filter, that way it'd output more specific markup like this.

@westonruter
Copy link
Contributor Author

  • Can you have a look in your settings of Yoast SEO and tell me what's under SEO -> Search Appearance -> General (the first tab) -> Knowledge Graph & Schema.org ? It's weird to see no Person or Organization output in your example output as our code basically always has at least one of those.

image

@westonruter
Copy link
Contributor Author

  • The image attribute in the pull you refer to seems to throw an array of images into a single image attribute. If I look at the Schema definition of BlogPosting, Article or basically any Thing, image can either be a full imageObject or just a URL. Putting in an array of URLs seems to me as to be stretching the specification.

    Wouldn't it be better if we change that to be 4 image attributes with 4 URLs or, even better, fully expanded imageObject's? Then the size of the images would be in the actual metadata, seems much more sensible and reusable web-wide to me, but I'm not sure whether that'll be parsed correctly on Google's end?

That's a good question. Actually, Google Search recommends an array of image URLs here:

image

I'm not sure if this means Google Search is recommending something that is not part of the Schema.org spec, but that's what it looks like.

@westonruter
Copy link
Contributor Author

  • Your default type is BlogPosting apparently in AMP Stories, with your pull it becomes a WebPage when Yoast SEO is on, I'm not sure that'll work the same way, should we just keep it BlogPosting? We could also choose to make it an Article, simply by using the wpseo_schema_article_post_type filter, that way it'd output more specific markup like this.

The AMP plugin's default Schema.org metadata uses this logic for singular queries to determine the @type:

'@type' => is_page() ? 'WebPage' : 'BlogPosting'

I don't know the nuances of Article vs BlogPosting, but yes, Yoast SEO should be outputting something other than WebPage. In c95fc45 I've made it output BlogPosting.

@@ -150,6 +150,9 @@ private function determine_page_type() {
case is_archive():
$type = 'CollectionPage';
break;
case is_singular( 'amp_story' ):
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having this in the yoast plugin, why not do this in the AMP plugin using the wpseo_schema_webpage_type filter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that filter is specific to Yoast. The AMP plugin doesn't generally have plugin-specific code. There was a bunch of Jetpack code but most of it has been moved to Jetpack.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the plugin specific code lives in yoast / jetpack instead of the other way around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. That's the scalable way to add AMP compatibly to the ecosystem, if thenesa and plugins take the responsibility of AMP compatibility.

@spacedmonkey
Copy link
Contributor

@westonruter Should this PR be moved from a WIP to a real PR so it can move forward. Willing to pick the work.

@westonruter
Copy link
Contributor Author

@spacedmonkey The PR is in a Draft state because of the outstanding questions/issues above. In other words, it's not ready to merge. I can add you to my fork so you can push directly to the branch for this PR if you like.

@spacedmonkey
Copy link
Contributor

@westonruter Unless you want to try and get this across the line yourself. I will to help out, specially if you have ideas, will to do the work and test it.

@westonruter
Copy link
Contributor Author

@spacedmonkey I invited you to my fork. Have at it!

@spacedmonkey
Copy link
Contributor

Related: #12864

@spacedmonkey
Copy link
Contributor

I made a small change to @westonruter pull request, that does the following.

  • Add article schema to amp stories.
  • Check to see if is_singular('amp_story') && function_exists( 'amp_get_schemaorg_metadata' )
  • Merge values in article schema with data returned from amp_get_schemaorg_metadata.

I must admit, I don't love the function exists, as it feels like it should be a better way of doing this. But it is a good starting off point.

This is the resulting ld

{"@context":"https://schema.org","@graph":[{"@type":"Organization","@id":"http://local.wordpress.test/#organization","name":"Spacedmonkey","url":"http://local.wordpress.test/","sameAs":[],"logo":{"@type":"ImageObject","@id":"http://local.wordpress.test/#logo","url":"http://local.wordpress.test/wp-content/uploads/2019/09/Screenshot-from-2019-08-29-13-10-32.png","width":807,"height":435,"caption":"Spacedmonkey"},"image":{"@id":"http://local.wordpress.test/#logo"}},{"@type":"WebSite","@id":"http://local.wordpress.test/#website","url":"http://local.wordpress.test/","name":"local.wordpress.test","publisher":{"@id":"http://local.wordpress.test/#organization"},"potentialAction":{"@type":"SearchAction","target":"http://local.wordpress.test/?s={search_term_string}","query-input":"required name=search_term_string"}},{"@type":"ImageObject","@id":"http://local.wordpress.test/blog/stories/auto-draft/#primaryimage","url":"http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521.jpg","width":1200,"height":1600},{"@type":"WebPage","@id":"http://local.wordpress.test/blog/stories/auto-draft/#webpage","url":"http://local.wordpress.test/blog/stories/auto-draft/","inLanguage":"en-US","name":"This is a test - local.wordpress.test","isPartOf":{"@id":"http://local.wordpress.test/#website"},"primaryImageOfPage":{"@id":"http://local.wordpress.test/blog/stories/auto-draft/#primaryimage"},"datePublished":"2019-09-20T08:51:15+00:00","dateModified":"2019-09-20T08:51:27+00:00"},{"@type":"BlogPosting","@id":"http://local.wordpress.test/blog/stories/auto-draft/#article","isPartOf":{"@id":"http://local.wordpress.test/blog/stories/auto-draft/#webpage"},"author":{"@type":"Person","name":"admin"},"headline":"This is a test","datePublished":"2019-09-20T08:51:15+00:00","dateModified":"2019-09-20T08:51:27+00:00","commentCount":0,"mainEntityOfPage":"http://local.wordpress.test/blog/stories/auto-draft/","publisher":{"@type":"Organization","name":"local.wordpress.test","logo":"http://local.wordpress.test/wp-content/plugins/amp/assets/images/stories-editor/amp-story-fallback-wordpress-publisher-logo.png"},"image":["http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521-696x928.jpg","http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521-928x928.jpg","http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521-928x696.jpg","http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521.jpg"]},{"@type":["Person"],"@id":"http://local.wordpress.test/#/schema/person/e73ca4257cfe586215b10f601e1451ff","name":"admin","image":{"@type":"ImageObject","@id":"http://local.wordpress.test/#authorlogo","url":"http://1.gravatar.com/avatar/ad5dd285560221a7302608c74c690035?s=96&d=mm&r=g","caption":"admin"},"sameAs":[]}]}

@westonruter
Copy link
Contributor Author

This is the resulting ld

With pretty-printing:

{
    "@context": "https://schema.org",
    "@graph": [
        {
            "@id": "http://local.wordpress.test/#organization",
            "@type": "Organization",
            "image": {
                "@id": "http://local.wordpress.test/#logo"
            },
            "logo": {
                "@id": "http://local.wordpress.test/#logo",
                "@type": "ImageObject",
                "caption": "Spacedmonkey",
                "height": 435,
                "url": "http://local.wordpress.test/wp-content/uploads/2019/09/Screenshot-from-2019-08-29-13-10-32.png",
                "width": 807
            },
            "name": "Spacedmonkey",
            "sameAs": [],
            "url": "http://local.wordpress.test/"
        },
        {
            "@id": "http://local.wordpress.test/#website",
            "@type": "WebSite",
            "name": "local.wordpress.test",
            "potentialAction": {
                "@type": "SearchAction",
                "query-input": "required name=search_term_string",
                "target": "http://local.wordpress.test/?s={search_term_string}"
            },
            "publisher": {
                "@id": "http://local.wordpress.test/#organization"
            },
            "url": "http://local.wordpress.test/"
        },
        {
            "@id": "http://local.wordpress.test/blog/stories/auto-draft/#primaryimage",
            "@type": "ImageObject",
            "height": 1600,
            "url": "http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521.jpg",
            "width": 1200
        },
        {
            "@id": "http://local.wordpress.test/blog/stories/auto-draft/#webpage",
            "@type": "WebPage",
            "dateModified": "2019-09-20T08:51:27+00:00",
            "datePublished": "2019-09-20T08:51:15+00:00",
            "inLanguage": "en-US",
            "isPartOf": {
                "@id": "http://local.wordpress.test/#website"
            },
            "name": "This is a test - local.wordpress.test",
            "primaryImageOfPage": {
                "@id": "http://local.wordpress.test/blog/stories/auto-draft/#primaryimage"
            },
            "url": "http://local.wordpress.test/blog/stories/auto-draft/"
        },
        {
            "@id": "http://local.wordpress.test/blog/stories/auto-draft/#article",
            "@type": "BlogPosting",
            "author": {
                "@type": "Person",
                "name": "admin"
            },
            "commentCount": 0,
            "dateModified": "2019-09-20T08:51:27+00:00",
            "datePublished": "2019-09-20T08:51:15+00:00",
            "headline": "This is a test",
            "image": [
                "http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521-696x928.jpg",
                "http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521-928x928.jpg",
                "http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521-928x696.jpg",
                "http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521.jpg"
            ],
            "isPartOf": {
                "@id": "http://local.wordpress.test/blog/stories/auto-draft/#webpage"
            },
            "mainEntityOfPage": "http://local.wordpress.test/blog/stories/auto-draft/",
            "publisher": {
                "@type": "Organization",
                "logo": "http://local.wordpress.test/wp-content/plugins/amp/assets/images/stories-editor/amp-story-fallback-wordpress-publisher-logo.png",
                "name": "local.wordpress.test"
            }
        },
        {
            "@id": "http://local.wordpress.test/#/schema/person/e73ca4257cfe586215b10f601e1451ff",
            "@type": [
                "Person"
            ],
            "image": {
                "@id": "http://local.wordpress.test/#authorlogo",
                "@type": "ImageObject",
                "caption": "admin",
                "url": "http://1.gravatar.com/avatar/ad5dd285560221a7302608c74c690035?s=96&d=mm&r=g"
            },
            "name": "admin",
            "sameAs": []
        }
    ]
}

@spacedmonkey
Copy link
Contributor

I believe there are two solutions here.

  1. Do an array merge article ld json, as the fields are very similar. However, this isn't the cleanest, as both plugins are changed, these changes will need to be tested.
  2. A much simpler implementation, would add in another element to array in the get_graph_pieces method using the wpseo_schema_graph_pieces.

Both have they pros and cons. One is destructive change and the other is additive.

I am trying to understand why type BlogPosting was used, as Article seems better here.

@spacedmonkey
Copy link
Contributor

I believe there are two solutions here.

  1. Do an array merge article ld json, as the fields are very similar. However, this isn't the cleanest, as both plugins are changed, these changes will need to be tested.
  2. A much simpler implementation, would add in another element to array in the get_graph_pieces method using the wpseo_schema_graph_pieces.

Both have they pros and cons. One is destructive change and the other is additive.

I am trying to understand why type BlogPosting was used, as Article seems better here.

@jdevalk @westonruter Thoughts?

@Jared-Williams
Copy link

@spacedmonkey @westonruter Is there any update to this? Would be happy to test to help make opengraph metadata a reality on AMP stories

@westonruter
Copy link
Contributor Author

@Jared-Williams The current status is the outstanding question above. But by all means, do test and provide feedback! Thank you.

@jonoalderson
Copy link

I am trying to understand why type BlogPosting was used, as Article seems better here.

We can't guarantee that the Article is/should be a BlogPosting. That's why use Article, unless we have explicit context.

@jonoalderson
Copy link

  • Wouldn't it be better if we change that to be 4 image attributes with 4 URLs or, even better, fully expanded imageObject's? Then the size of the images would be in the actual metadata, seems much more sensible and reusable web-wide to me, but I'm not sure whether that'll be parsed correctly on Google's end?

Yes, we should use full imageObject markup.

@spacedmonkey
Copy link
Contributor

@jono-alderson Can you review the output from our PR and confirm you are happy with the output?

@jonoalderson jonoalderson marked this pull request as ready for review October 29, 2019 14:03
@jonoalderson
Copy link

Oops. Accidentally marked as ready for review. I don't appear to be able to undo that.

@westonruter
Copy link
Contributor Author

Yes, we should use full ImageObject markup.

@jono-alderson I checked with @flaviori who works on the Google Search integration and he says to use image as array, and that the image array should be URL strings not ImageObjects, so like:

"image": [
  "http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521-696x928.jpg",
  "http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521-928x928.jpg",
  "http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521-928x696.jpg",
  "http://local.wordpress.test/wp-content/uploads/2019/09/large-image-36521.jpg"
],

This is according to the Google Search recommendations, as referenced above: #13446 (comment)

@westonruter
Copy link
Contributor Author

In regards to image being an array, please also refer to: https://github.com/schemaorg/schemaorg/wiki/Examples:-Photos-and-Images

image

@jonoalderson
Copy link

The 'array of images' approach is technically valid, but not preferable, but a bunch of reasons. I'd rather we do this right, and use an array of fully described imageObjects.

@jonoalderson
Copy link

jonoalderson commented Oct 30, 2019

A further consideration in the meantime; the size/ratio (and content) requirements for the logo (attached to Organization) are different on AMP pages - and different again for AMP stories.

This may require some admin UI considerations on the AMP plugin side?

See https://developers.google.com/search/docs/data-types/article#logo-guidelines and https://amp.dev/documentation/components/amp-story/?referrer=ampproject.org#publisher-logo-src-guidelines

@jonoalderson
Copy link

Also, I'm taking a closer look at the JSON-LD above, and it looks like there are some oddities in the structure.

E.g.:

  • The blogPosting references (the stub of) a different Organization object from the one at http://local.wordpress.test/#organization (albeit with duplicate properties)
  • The maiEntityOfPage property is incorrectly referencing a URL, rather than referencing the WebPage by @id.
  • The post author isn't connected into the graph at all.

Perhaps you could take a look at our spec? See https://developer.yoast.com/features/schema/pieces/article/

@flaviori
Copy link

flaviori commented Oct 30, 2019

These are all very good points.

The 'array of images' approach is technically valid, but not preferable, but a bunch of reasons. I'd rather we do this right, and use an array of fully described imageObjects.

Yeah indeed this is not the cleanest approach. Ideally we would have had separate keys with separate names for each image, but the array of images was simpler and didn't require changes in the schema.

Regarding using ImageObject instead of URL, that seems reasonable, so Weston that's fine from a Search perspective.

However, I recommend avoiding specifying the height/width fields of ImageObject for a few reasons. In short, they trigger warnings and errors in several validation tools if the values don't match the actual image size. In practice, they have redundancy issues e..g, it's easy to update the image at the URL and forget to update the size. Unfortunately the truth that is not safe to trust them often and is preferred to fetch the image and check the actual size whenever possible. Note that Search will properly parse the ImageObect and just ignore the values, but for example SDTT may throw errors.
The array of ImageObject works:

KggV6y4iymm

A further consideration in the meantime; the size/ratio (and content) requirements for the logo (attached to Organization) are different on AMP pages - and different again for AMP stories.

This may require some admin UI considerations on the AMP plugin side?

See https://developers.google.com/search/docs/data-types/article#logo-guidelines and https://amp.dev/documentation/components/amp-story/?referrer=ampproject.org#publisher-logo-src-guidelines

AMP stories have different sizing requirements for both the main image and the logo. In particular, because these stories are often represented with a portrait full bleed preview card, the logo recommendation is square so that it can be used on top of the cover as opposed to landscape like regular AMP. Note that AMP results may and often do specify a square logo instead of landscape. Landscape is recommended for AMP, but square is also supported (although not properly documented ... sorry again).

tyYBMDeyqSS

I realize that some of these are sub-optimal. I guess a conversation for another day :).

@jonoalderson
Copy link

Thanks for your feedback, @flaviori!

I'm confident that we can accurately provide the height/width values. We check/calculate those with some robustness, and there's no native way in WordPress to 'replace' a file; changes to images always result in a new URL.

RE: Logos versions, those should probably be handled via a UI/configuration component in the AMP plugin, then conditionally selected as the imageObject used in/for the organization's logo.

@westonruter
Copy link
Contributor Author

I'm confident that we can accurately provide the height/width values. We check/calculate those with some robustness, and there's no native way in WordPress to 'replace' a file; changes to images always result in a new URL.

What about plugins that filter the image URLs? Jetpack's Photon, for example, constrains the max dimensions for images used when it rewrites image URLs to point to its CDN.

@jonoalderson
Copy link

jonoalderson commented Oct 31, 2019

See https://developer.yoast.com/features/schema/pieces/image/, width and height are only output when (both) are known - and we should still output an imageObject so that we can cross-reference by ID, etc.

@jdevalk
Copy link
Contributor

jdevalk commented Jun 24, 2020

Hey @westonruter what's the status here? Our code has moved around a bit, I actually think this should be here:

https://github.com/Yoast/wordpress-seo/blob/trunk/src/integrations/third-party/amp.php

@swissspidy
Copy link
Contributor

@jdevalk Yeah I think this PR can be closed. We'll open a new one for integration with the superseding Web Stories plugin, to live at integrations/third-party/web-stories.php. Does that sound good to you?

@jdevalk
Copy link
Contributor

jdevalk commented Jun 24, 2020

Sounds perfect to me! Closing.

@jdevalk jdevalk closed this Jun 24, 2020
@swissspidy swissspidy mentioned this pull request Jun 24, 2020
6 tasks
@swissspidy
Copy link
Contributor

New follow-up PR: #15532

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants