Set featured image if none exists, based on adding other images by kienstra · Pull Request #1998 · ampproject/amp-wp

kienstra · 2019-03-20T12:27:44Z

If no featured image exists, set it based on:

Adding a background image to a page block
In the Image block, selecting or uploading an image via the Media Library
In the Image block, clicking 'Upload' and uploading an image

Closes #1954

@todo

But only if there isn't a featured image. @todo: consider this for the image block.

On selecting an image in the Image block's Media Library, check if there's a featured image. If not, set this as the featured image. This doesn't apply to the 'Upload' or 'Insert from URL' buttons.

When clicking 'Upload' and uploading an image, set it as the featured image if there is none.

assets/src/blocks/amp-story/edit.js

There's now a handler for the MediaUpload component, so that it sets the featured image if there is none. So this logic isn't needed anymore.

When setting the featured image via the Imge block or the Story page's 'Background Media' control, hide the admin notice that there is no featured image.

Mainly copy the logic from amp-block-validation.js for the module with-featured-image-notice.js.

swissspidy · 2019-03-21T09:07:43Z

Should this include some logic to set the featured image only when the uploaded image is large enough?

swissspidy · 2019-03-21T18:38:27Z

For reference:

Each poster image should meet the recommended minimum size:

Portrait: 696px x 928px
Landscape: 928px x 696px
Square: 928px x 928px

kienstra · 2019-03-21T21:12:24Z

Poster Image

Hi @swissspidy,
Good point, this PR should probably ensure that the image meets the minimum size.

… featured image As Pascal mentioned, it'll be good to ensure that an image has the correct poster image dimensions before automatically setting it as the featured image.

kienstra · 2019-03-22T02:20:10Z

Image Dimensions Now Verified

Hi @swissspidy,
Good suggestion that this check for the minimum poster dimensions.

Based on the dimensions you shared, the following images are accepted:
https://lorempixel.com/669/928
https://lorempixel.com/928/669
https://lorempixel.com/928/928

While these dimensions won't be:
https://lorempixel.com/700/700
https://lorempixel.com/900/927

As mentioned in Issue 1954, this adds a notice on clicking 'Publish' if there is no featured image.

kienstra · 2019-03-22T05:22:47Z

Pre-publish Notice

There's now a notice on clicking 'Publish' when an AMP Story doesn't have a featured image:

There was only a conflic in: amp-story-editor-blocks.js in the import statements.

kienstra · 2019-03-22T06:02:33Z

assets/js/amp-block-validation.js

-		/**
-		 * Handles admin notices for AMP Stories.
-		 */
-		handleAMPStoryChange: function handleAMPStoryChange() {


This notice at the top of the entire editor is replaced by a notice above the Featured Image control in with-featured-image-notice.js.

One is a typo, using 'not' instead of 'no'. The other is a change from 'display' to 'this displays'.

kienstra · 2019-03-22T06:24:22Z

Request For Another Review

Hi @swissspidy,
Thanks for your suggestions. Could you please review this again?

Here's the expected behavior:

On adding an image in the following places, this sets it as the featured image if none exists:

a. The Image block's 'Media Library' or 'Upload' buttons:

b. The 'Select Media' button under'Background Media':

Moves the notice for no featured image from the top of the block editor to above the 'Featured Image' section in the Inspector:

On clicking 'Publish' when there's no featured image, displays a notice in the pre-publish panel

swissspidy · 2019-03-22T09:39:44Z

assets/src/components/with-placeholder-featured-image.js

+				}
+
+				// Conditionally set the uploaded image as the featured image.
+				const media = wp.data.select( 'core' ).getMedia( selectedBlock.attributes.id );


Suggested change

const media = wp.data.select( 'core' ).getMedia( selectedBlock.attributes.id );

const media = select( 'core' ).getMedia( selectedBlock.attributes.id );

You already import select at the top 🙂

Good idea, I committed your suggestion.

swissspidy · 2019-03-22T09:40:38Z

assets/src/components/with-placeholder-featured-image.js

+		 *
+		 * If there's no featured image, set this image as the featured image.
+		 * This component unmounts right after uploading an image.
+		 * But this needs to be delayed, as there's an apiFetch() call that gets the id.


Where is that apiFetch() call happening? Can we somehow avoid using delay() by awaiting the result of that or something?

Good question, I'll look at this more. The apiFetch() call happens when the MediaUploader calls createMediaFromFile().

7a3df1a prevents using delay() by instead filtering BlockEdit.

As Pascal mentioned, it already imports '@wordpress/data` at the top Co-Authored-By: kienstra <kienstraryan@gmail.com>

googlebot · 2019-03-22T16:43:50Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-22T18:20:27Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

I introduced a regression where this didn't work anymore, as it uses attributes.mediaId, instead of attributes.id

As Weston suggested, accept the dimensions as parameters. This will allow it to be used for the post type of 'post' also.

As Weston mentioned, these should also be included. So set these if they exist.

kienstra · 2019-03-23T05:10:17Z

Placeholder Poster Image

Hi @westonruter,

So to ensure that this doesn't happen, a placeholder image...should be shown

Sounds good, would you mind if I asked @dawidmlynarz to create an image for that?

kienstra · 2019-03-23T05:10:53Z

Additional Poster Attributes

Hi @westonruter,

In addition to poster-portrait-src there should be attributes provided for poster-landscape-src and poster-square-src, per the docs.

Good point. How does c2381f2 look? The poster-square-src and poster-landscape-src attributes are now added:

Abstract this into the function getFeaturedImageNotice() this can be used for the post featured image, also.

Using the logic for AMP stories, add a notice to normal posts if the featured image has a width less than 1200px.

Pass different arguments, as the post requires that a featured image have a width of 1200px.

Including adding JSDoc tags, and spaces before and after brackets.

This might be the cause of a failed Travis build.

kienstra · 2019-03-25T00:07:29Z

Post Featured Image Notice

This adds the following to a normal post:

A notice above the 'Featured Image' panel if it doesn't exist or is the wrong dimension
A notice on clicking 'Publish' if the featured image doesn't exist or has the wrong width

kienstra · 2019-03-25T00:08:57Z

Also Adds Size Validation To AMP Story

This also adds the invalid size notices shown above to AMP stories. Before, there were only notices if the featured images didn't exist at all.

assets/src/amp-block-editor.js

There were some functions that had no JSDocs, so add some.

kienstra · 2019-03-25T00:42:17Z

assets/src/amp-block-editor.js

+ * @return {boolean} Whether the media has the minimum dimensions.
+ */
+const hasMinimumFeaturedImageWidth = ( media ) => {
+	return ( media.width && media.width >= 1200 );


If it's alright, I used an entirely different function for normal posts. This function for normal posts is simpler than the function to check that an AMP Story's featured image has the right dimensions.

These have dimensions of 696 and 928, so update the image sizes.

Add a simple .jpg for the poster-portrait-src, in case there is no featured image. There will still be notices in the block editor, but at least the AMP Story will display. Before, it didn't display at all with no featured image.

kienstra · 2019-03-27T04:30:20Z

Request For Another Review

Hi @swissspidy,
Hope you're doing great.

When you have a chance, could you please review this PR again?

The main changes lately are adding notices if the featured image sizes are wrong in AMP Stories and posts, and a fallback image if there's no featured image.

Thanks, Pascal! And congratulations on speaking at WordCamp Europe.

It’s not actually a UI component, just a string, which is better suited here.

It’s not actually a UI component, just a boolean callback, which is better suited here.

swissspidy

Let's ship it! 🚢

I still have to get used to the fact that any big enough image I add to the story suddenly ends up as the featured image. We should document this well.

The other really minor thing is that the default fallback image does not translate well. One does never really see it while writing anyway, so I guess it's fine.

Further polishing can be done in separate PRs if needed.

kienstra · 2019-03-28T20:09:55Z

Thanks, @swissspidy! You're right, we should document the fact that selecting a big enough image makes in the featured image (poster-portrait-src).

kienstra added 3 commits March 15, 2019 00:39

On selecting 'Background Media,' set it as the featured image

dab51d4

But only if there isn't a featured image. @todo: consider this for the image block.

Conditionally set the Image block's image as the featured image

7fb77d9

On selecting an image in the Image block's Media Library, check if there's a featured image. If not, set this as the featured image. This doesn't apply to the 'Upload' or 'Insert from URL' buttons.

In the Image block, conditionally set the featured image from uploading

a515779

When clicking 'Upload' and uploading an image, set it as the featured image if there is none.

googlebot added the cla: yes Signed the Google CLA label Mar 20, 2019

swissspidy reviewed Mar 20, 2019

View reviewed changes

assets/src/blocks/amp-story/edit.js Outdated Show resolved Hide resolved

kienstra added 3 commits March 20, 2019 18:00

Remove logic in amp-story/edit.js module to set featured image

2237ad9

There's now a handler for the MediaUpload component, so that it sets the featured image if there is none. So this logic isn't needed anymore.

Remove the notice for the missing featured image in some cases

a11cf19

When setting the featured image via the Imge block or the Story page's 'Background Media' control, hide the admin notice that there is no featured image.

Move the featured image notice from the top of the editor to the sidebar

daa1ac8

Mainly copy the logic from amp-block-validation.js for the module with-featured-image-notice.js.

swissspidy added the AMP-Stories label Mar 21, 2019

Check that an image has the proper dimension before setting it as the…

87c0e98

… featured image As Pascal mentioned, it'll be good to ensure that an image has the correct poster image dimensions before automatically setting it as the featured image.

Add a notice on publishing if there is no featured image

7518286

As mentioned in Issue 1954, this adds a notice on clicking 'Publish' if there is no featured image.

Merge in amp-stories-redux, and resolve conflict

aa4f18c

There was only a conflic in: amp-story-editor-blocks.js in the import statements.

kienstra commented Mar 22, 2019

View reviewed changes

Fix errors in JSDoc comments

24a399a

One is a typo, using 'not' instead of 'no'. The other is a change from 'display' to 'this displays'.

kienstra changed the title ~~[WIP] Set featured image if none exists, based on adding other images~~ Set featured image if none exists, based on adding other images Mar 22, 2019

swissspidy reviewed Mar 22, 2019

View reviewed changes

Apply Pascal's suggestion of removing wp.data before select

358c633

As Pascal mentioned, it already imports '@wordpress/data` at the top Co-Authored-By: kienstra <kienstraryan@gmail.com>

googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Mar 22, 2019

westonruter added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Mar 22, 2019

kienstra added 3 commits March 22, 2019 22:30

Restore the 'Background Media' button setting feat image

a97bb47

I introduced a regression where this didn't work anymore, as it uses attributes.mediaId, instead of attributes.id

Prepare hasMinimumDimensions() for use in other contexts

22b0f54

As Weston suggested, accept the dimensions as parameters. This will allow it to be used for the post type of 'post' also.

Ouptput landscape and square image src values

c2381f2

As Weston mentioned, these should also be included. So set these if they exist.

kienstra added 5 commits March 24, 2019 15:51

Refactor story featured image notice to allow reuse in posts

cfa19df

Abstract this into the function getFeaturedImageNotice() this can be used for the post featured image, also.

Add featured image width validation to normal posts

e1fe788

Using the logic for AMP stories, add a notice to normal posts if the featured image has a width less than 1200px.

Reuse the pre-publish notice for normal posts

52756b2

Pass different arguments, as the post requires that a featured image have a width of 1200px.

Address ESLint issues in failed Travis build

8227a4c

Including adding JSDoc tags, and spaces before and after brackets.

Add a semicolon to address ESLint issue

f93e584

This might be the cause of a failed Travis build.

kienstra commented Mar 25, 2019

View reviewed changes

assets/src/amp-block-editor.js Show resolved Hide resolved

Add JSDocs, including @param tags

df50f11

There were some functions that had no JSDocs, so add some.

kienstra commented Mar 25, 2019

View reviewed changes

kienstra added 2 commits March 25, 2019 20:14

Update image sizes to the spec for poster-portrait, landscap, and src

f77cadc

These have dimensions of 696 and 928, so update the image sizes.

Add a fallback image in case there is no featured image

d8add66

Add a simple .jpg for the poster-portrait-src, in case there is no featured image. There will still be notices in the block editor, but at least the AMP Story will display. Before, it didn't display at all with no featured image.

kienstra assigned swissspidy Mar 27, 2019

swissspidy added 3 commits March 28, 2019 14:46

Move getFeaturedImageMessage to helpers

daec243

It’s not actually a UI component, just a string, which is better suited here.

Move hasMinimumStoryPosterDimensions to helpers as well

dfad3cb

It’s not actually a UI component, just a boolean callback, which is better suited here.

Make notices non-dismissible

c521ff7

swissspidy approved these changes Mar 28, 2019

View reviewed changes

swissspidy merged commit 86892f6 into amp-stories-redux Mar 28, 2019

swissspidy deleted the add/automatic-featured-image branch March 28, 2019 14:27

This was referenced Mar 29, 2019

Improve featured image notices #2038

Merged

Allow cropping of the AMP Story featured image #2031

Merged

westonruter added this to the v1.2 milestone May 21, 2019

westonruter mentioned this pull request Aug 28, 2019

Supply placeholder poster image for videos in AMP Stories #3114

Closed

	const media = wp.data.select( 'core' ).getMedia( selectedBlock.attributes.id );
	const media = select( 'core' ).getMedia( selectedBlock.attributes.id );

Conversation

kienstra commented Mar 20, 2019

Uh oh!

Uh oh!

swissspidy commented Mar 21, 2019

Uh oh!

swissspidy commented Mar 21, 2019

Uh oh!

kienstra commented Mar 21, 2019

Uh oh!

kienstra commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kienstra commented Mar 22, 2019

Uh oh!

kienstra Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

kienstra commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swissspidy Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

kienstra Mar 23, 2019

Choose a reason for hiding this comment

Uh oh!

swissspidy Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

kienstra Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

kienstra Mar 23, 2019

Choose a reason for hiding this comment

Uh oh!

googlebot commented Mar 22, 2019

Uh oh!

googlebot commented Mar 22, 2019

Uh oh!

kienstra commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kienstra commented Mar 23, 2019

Uh oh!

kienstra commented Mar 25, 2019

Uh oh!

kienstra commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kienstra Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kienstra commented Mar 27, 2019

Uh oh!

swissspidy left a comment

Choose a reason for hiding this comment

Uh oh!

kienstra commented Mar 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kienstra commented Mar 22, 2019 •

edited

Loading

kienstra commented Mar 22, 2019 •

edited

Loading

kienstra commented Mar 23, 2019 •

edited

Loading

kienstra commented Mar 25, 2019 •

edited

Loading

kienstra Mar 25, 2019 •

edited

Loading