Improve featured image notices#2038
Conversation
|
Actually, doesn't this warning make sense for regular posts as well? If you don't have a featured image set then Google Search Console issues warnings about not having an image. Maybe a notice would be more appropriate than a warning, since it is a hard error for AMP Stories but only a warning for other AMP pages. A warning should be shown regardless if the selected image does not have adequate dimensions to satisfy the Google Search requirements, as again, a GSC will issue warnings. |
|
Good point. The wording should be a bit different then. I mainly flagged this here because it's a change from previous behavior. A notice for regular AMP pages could make sense. |
|
See #1812 for how the notice could apply for non-story posts. |
|
What about this:
|
|
Humm. I don't think that is quite accurate. The page will still be indexed and appear in search results. I think it's just that it won't appear in the Search Carousel. Could you check with the Search team there as for why specifically this is required?
But there are other reasons to encourage the featured image to be supplied, like in social media shares. But the other reason to include the image would be to avoid GSC issuing a warning.
All this being said, what if someone embeds the image in the post content instead of adding as a featured image. I've seen that before and the AMP plugin actually has some detection for that I believe. I'll check.
|
I got this reversed actually. The AMP plugin in the Classic/Reader mode only, will skip outputting the featured image in the template if that same image exists inside the post content: amp-wp/includes/templates/class-amp-post-template.php Lines 344 to 351 in ffd66f6 The related issue is then #1845 which is to automatically extract image for Schema.org metadata from content if no featured image selected. This is actually similar to what is happening now for the Story editor actually. But this could be done at the PHP side of things, to rather scrape the content for a suitable image to use for Schema.org metadata if none was supplied. This is something that Jetpack is already doing. In any case, let's say that the AMP plugin incorporates that Jetpack logic to scrape the content for an image to use for the Schema.org metadata. Then this notice wouldn't need to be displayed if there was such a suitable image in the content. All of this being said (again), I suppose this is perhaps out of scope for what needs to be addressed first. For now, we can just assume the featured image is the only source for the Schema.org metadata. In that case, we should have a notice like you have depicted, though the text should reflect that it will help the post appear optimally when shared and when it is shown in search results. Does Yoast have anything in it to encourage the supplying of the featured image? Wonder if we might be duplicating a warning. |
Yoast Glue has the option to set default featured image, however, it doesn't seem to display a warning. |
Interesting. Is a PHP solution actually preferable for Stories as well?
Will improve the wording 👍 |
|
I didn't want to make the notice unnecessarily long by adding specific examples. I ended up with Screenshots:
|
assets/src/helpers.js
Outdated
| * @return {boolean} Whether the media has the minimum dimensions. | ||
| */ | ||
| export const hasMinimumFeaturedImageWidth = ( media ) => { | ||
| return ( media.width && media.width >= 1200 ); |
There was a problem hiding this comment.
The 1200 value is being used for the width, but there are more requirements than this we should also account for. Namely:
- Images should be at least 1200 pixels wide. ✅
- For best results, provide multiple high-resolution images (minimum of 800,000 pixels when multiplying width and height) with the following aspect ratios: 16x9, 4x3, and 1x1.
- Images must be in .jpg, .png, or .gif format.
So we should also address these later.
I see that hasMinimumStoryPosterDimensions is actually accounting for more of its requirements, so that is good:
Each poster image should meet the recommended minimium size:
- Portrait: 696px x 928px
- Landscape: 928px x 696px
- Square: 928px x 928px
Nevertheless, it is good that we are requiring 1200 for the featured image since then it can be re-used for both the poster and the Schema.org metadata. The cropping I believe is handled elsewhere.
There was a problem hiding this comment.
We should also update the Schema.org markup to output multiple images in the desired aspect ratios:
{
"@context": "https://schema.org",
"@type": "NewsArticle",
"image": [
"https://example.com/photos/1x1/photo.jpg",
"https://example.com/photos/4x3/photo.jpg",
"https://example.com/photos/16x9/photo.jpg"
]
}There was a problem hiding this comment.
Nevertheless, it is good that we are requiring 1200 for the featured image since then it can be re-used for both the poster and the Schema.org metadata.
There are two different requirements/checks:
- 696x928 for AMP Stories
- 1200 minimum width for regular posts
Did you mean that for AMP Stories we should also require 1200 minimum width?
There was a problem hiding this comment.
Oh, yes, actually. Sorry, I rewrote that comment a couple times as I discovered some more things. So since there is a separate function specifically for stories, then the 1200 constraint isn't applying. So yes, we should also enforce AMP stories gave a featured image of at least 1200px in order to satisfy the poster requirements and the metadata requirements.
There was a problem hiding this comment.
Cool, makes sense. I'll bump the width requirement in hasMinimumStoryPosterDimensions then.
|
I checked with Jon Newmuis about the recommended image sizes on the amphtml Slack and he said:
|
|
I really want to move this forward a bit as it is quite a big refactor at this point and has implications for #2176 as well due to the shared components. Separating featured image and poster image sounds interesting, and I guess it makes sense. Would make many things easier. |
|
I've fixed the issues with the featured image cropping tool, ensuring that a cropped image is not distorted. Also, the media library now prevents a user from selecting an image that is smaller than the minimum dimensions:
Now that the cropper is fixed, we now need to whether this featured image should be also used as the poster, and if so, whether a different crop is needed. Also need to make sure that this tool works on non-story posts. |
|
|
||
| /** | ||
| * Gets a wrapped version of a block's edit component that conditionally sets the featured image. | ||
| * Gets a wrapped version of a block's edit component that conditionally sets the featured image (only for AMP Story posts). |
There was a problem hiding this comment.
This needs to be applicable to non-story posts as well.
|
For a story I selected the Bison featured image and selected this crop: And this results in a story with this markup: <amp-story standalone
publisher-logo-src="https://wordpressdev.lndo.site/content/uploads/2019/02/cropped-google-logo-1-3.png"
publisher="WordPress Develop"
title="Test"
poster-portrait-src="https://wordpressdev.lndo.site/content/uploads/2019/04/cropped-American_bison_k5680-1-35-696x928.jpg"
poster-square-src="https://wordpressdev.lndo.site/content/uploads/2019/04/cropped-American_bison_k5680-1-35-928x928.jpg"
poster-landscape-src="https://wordpressdev.lndo.site/content/uploads/2019/04/cropped-American_bison_k5680-1-35-928x696.jpg">So given that the originally-selected featured image was a landscape image cropped at (1200×928): The The The In short, this all looks good for a story. Every image meets the requirements. The auto-generated crops from the featured image work well, as long as the user decides to center the image. |
|
@swissspidy OK, I think this is good to merge. Please review my latest changes to the JS as you probably have a better way to organize. |
|
We may need to revisit whether it is a good idea to present the cropping tool for non-story posts. Oh, and one more thing which may need your touch: If I load the story editor in code editor mode, there is a JS error:
This may not be related to the changes here at all, but part of what you are working on in #2117. |
It was a bit unexpected, and perhaps it can be a bit unexpected for others too. Let's keep it in for now for some testing.
While I added a fix in #2117, this was there already before, I think. |
| } | ||
|
|
||
| onSelect( data ); | ||
| onSelect( data ); // @todo Does this do anything? |
There was a problem hiding this comment.
Well it passes the information to whatever onSelect callback prop that was passed to the component.
|
The notices seem to work well. Let's merge this now and re-organize things when needed. Separate fields for poster image and featured image can be handled in separate PRs. |
This is a follow-up bug fix for #1998.
Unfortunately I only realized after merging that the "Selecting a featured image is required." was being displayed even for regular posts, not just stories.
However, for single posts we only want a notice when an image is set but too small.
With the existing functions it didn't seem to be that clear how to change the logic without making it harder to read. So I ended up going down the rabbit hole and turning them into separate components and higher-order components at the same time. This way they fit a bit better into the rest of the code base there I think.