Add PostFeaturedImage to PrePublishPanel #6563

arthur791004 · 2021-08-25T10:56:28Z

Summary

The UI for the Featured Image section of the sidebar should be the same in the two scenarios pictured above.

Currently, we can set featured image directly in Post Setting section. If we don't set any featured image and want to publish the post, we will see the notice and encourage to set the featured image. However, we cannot set the featured image immediately at that point. So, adding PostFeaturedImage to PrePublishPanel to make user able to set it immediately.

Screenshots

post-featured-image.mov

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

CLAassistant · 2021-08-25T10:56:32Z

All committers have signed the CLA.

pierlon · 2021-08-26T02:53:10Z

Hey @arthur791004, thanks for creating this PR. I'm not sure if it was ever intended for one to be able to upload a featured image from the pre-publish panel but given that it is a pre-publish check it would make sense.

cc @westonruter - I'm adding this to the v2.1.4 milestone and classifying it as a bug.

pierlon · 2021-08-26T02:56:21Z

assets/src/common/components/pre-publish-panel.js

-					);
-				} ) }
-			</Notice>
+			<PostFeaturedImage


I think it would be a better user experience to let the featured image check stay in the pre-publish panel after they've selected an image, rather than exiting the panel to edit it in the post settings panel.

Sounds good! Let me think about how to do it

I've made the check always staying in the pre-publish panel and also update the recorded video!

arthur791004 · 2021-08-26T03:00:58Z

@pierlon Thanks for your reply! Currently, I'm not very for sure but I think the reason might be that this plugin only adds the notice message to Post Setting section, so only adding the notice to PrePublish panel seems to make sense. The potential problem might be if there is new version for PostFeaturedImage, then this plugin should also update to make it consistent.

pierlon · 2021-08-26T23:41:17Z

assets/src/common/components/pre-publish-panel.js

+	const noticeUI = errors ? (
+		<Notice
+			status={ required ? 'warning' : 'notice' }
+			isDismissible={ false }
+		>
+			{ errors.map( ( errorMessage, index ) => {
+				return (
+					<p key={ `error-${ index }` }>
+						{ errorMessage }
+					</p>
+				);
+			} ) }
+		</Notice>
+	) : null;


I just noticed that since the plugin is filtering the PostFeaturedImage component to add the error notice in the Post Settings, we don't need to recreate the notice UI here and instead use the PostFeaturedImage component as is. The parameter annotations for this component (PrePublishPanel) and propTypes object would have to also be adapted of course.

Suggested change

const noticeUI = errors ? (

<Notice

status={ required ? 'warning' : 'notice' }

isDismissible={ false }

>

{ errors.map( ( errorMessage, index ) => {

return (

<p key={ `error-${ index }` }>

{ errorMessage }

</p>

);

} ) }

</Notice>

) : null;

Great 👍 I've removed lots of code that doesn't need now!

pierlon · 2021-08-26T23:41:57Z

assets/src/common/components/pre-publish-panel.js

-					);
-				} ) }
-			</Notice>
+			<PostFeaturedImage noticeUI={ noticeUI } />


Suggestion to go along with comment above:

Suggested change

<PostFeaturedImage noticeUI={ noticeUI } />

<PostFeaturedImage />

pierlon · 2021-08-26T23:45:44Z

@arthur791004 Could you also adapt the \Test_AMP_Post_Meta_Box::test_enqueue_block_assets test to include wp-editor as a script dependency? It would essentially look like this:

diff --git a/tests/php/test-class-amp-meta-box.php b/tests/php/test-class-amp-meta-box.php
--- a/tests/php/test-class-amp-meta-box.php	(revision 4d9cf3e11d131064ec989986f930c0d3c56d9977)
+++ b/tests/php/test-class-amp-meta-box.php	(date 1630021426599)
@@ -166,6 +166,7 @@
 				'wp-compose',
 				'wp-data',
 				'wp-edit-post',
+				'wp-editor',
 				'wp-element',
 				'wp-hooks',
 				'wp-i18n',

…turedImage

assets/src/common/components/pre-publish-panel.js

…ed image

pierlon · 2021-08-27T05:30:19Z

I've updated some of the component docblocks to reflect the changes here in a3f904a. Also, in ac183ea I've refactored the way the featured image notice is rendered so that a warning notice is shown when there are validation errors.

codecov · 2021-08-27T05:36:19Z

Codecov Report

Merging #6563 (1fb801e) into develop (ab5a1e7) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             develop    #6563      +/-   ##
=============================================
- Coverage      75.77%   75.77%   -0.01%     
  Complexity      6016     6016              
=============================================
  Files            239      239              
  Lines          18896    18894       -2     
=============================================
- Hits           14319    14317       -2     
  Misses          4577     4577

Flag	Coverage Δ
javascript	`78.86% <100.00%> (-0.06%)`	⬇️
php	`75.64% <ø> (ø)`
unit	`75.64% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
assets/src/common/helpers/index.js	`91.22% <100.00%> (-0.30%)`	⬇️

pierlon

@arthur791004 Thank you for submitting this PR and spending the time to work on it 🙇.

LGTM!

arthur791004 · 2021-08-27T05:57:14Z

@pierlon Thanks for your quick reply and help to update the comment 👍

westonruter · 2021-08-27T15:14:50Z

assets/src/common/helpers/index.js

-		if ( required ) {
-			return [ __( 'Selecting a featured image is required.', 'amp' ) ];
-		}
-
-		return [ __( 'Selecting a featured image is recommended for an optimal user experience.', 'amp' ) ];
+		return [ __( 'Selecting a featured image is required.', 'amp' ) ];


Humm. Why was the message made unconditionally required? If the required prop is removed, the resulting message should rather be Selecting a featured image is recommended for an optimal user experience.. Although this message as well should be improved (although not necessarily at this time) per #6075.

In other words, should these lines not be instead replaced with:

return [ __( 'Selecting a featured image is recommended for an optimal user experience.', 'amp' ) ];

There are many scenarios where no featured image neither available nor desired when someone publishes a post, so we shouldn't mandate Google Search's guidelines as requirements.

Indeed, the Featured Image is used as the image in the Schema.org metadata, and the Guidelines make a post eligible to show up with structured data in Search results. The post will still show up in search results without the extra metadata, although it won't be as well presented.

The idea here is to only let this function validate the image and return a set of validation errors. The text Selecting a featured image is recommended for an optimal user experience. is not necessarily an error but a reconmmendation, so I removed it from here.

Instead, the component where this function is being used does a check to see if media is nullable, and if so uses the text Selecting a featured image is recommended for an optimal user experience.. See https://github.com/arthur791004/amp-wp/blob/1fb801ed0a1fceaffec5087327c391e0f752782d/assets/src/common/components/higher-order/with-featured-image-notice.js#L60-L66.

if ( ! media ) { const message = __( 'Selecting a featured image is recommended for an optimal user experience.', 'amp' ); noticeUI = createNoticeUI( [ message ], 'notice' ); } else { const errorMessages = validateFeaturedImage( media, getMinimumFeaturedImageDimensions() ); noticeUI = errorMessages ? createNoticeUI( errorMessages, 'warning' ) : null; }

Oh, so this “required” message is actually never shown in the plugin currently but is included in validateFeaturedImage() here for completeness?

delawski

LGTM!

The only thing that looks a bit off to me is the stats: over 4k lines added. Most of that is the package-lock.json. I guess it's because of adding the @wordpress/editor package to the project.

pierlon · 2021-08-30T16:10:38Z

Uninstalled then reinstalled the @wordpress/editor package and i can confirm it results in the same package-lock.json.

westonruter · 2021-08-30T16:14:56Z

@pierlon This doesn't cherry-pick cleanly into 2.1. Would you open a PR onto that branch with the conflicts resolved?

pierlon · 2021-08-30T16:15:14Z

Sure.

westonruter · 2021-08-30T22:53:02Z

QA Passed

Before	After

And I was able to set the featured image directly from the pre-publish panel:

arthur791004 requested a review from pierlon as a code owner August 25, 2021 10:56

arthur791004 force-pushed the add/post-featured-image-in-pre-publish-panel branch from cb979f8 to 6a58cb0 Compare August 25, 2021 10:57

arthur791004 mentioned this pull request Aug 25, 2021

Editor settings sidebar: Featured image missing grey placeholder box Automattic/wp-calypso#55566

Open

Add PostFeaturedImage to PrePublishPanel

21498ed

arthur791004 force-pushed the add/post-featured-image-in-pre-publish-panel branch from 6a58cb0 to 21498ed Compare August 26, 2021 02:32

pierlon added the Editor label Aug 26, 2021

pierlon added this to the v2.1.4 milestone Aug 26, 2021

pierlon added the Bug Something isn't working label Aug 26, 2021

pierlon reviewed Aug 26, 2021

View reviewed changes

Keep featured image check in pre-publish panel after selected an image

4d9cf3e

pierlon suggested changes Aug 26, 2021

View reviewed changes

arthur791004 added 2 commits August 27, 2021 11:05

Fix tests

88edc04

Simplify PrePublishPanel because we already applied filter to PostFea…

3938ae8

…turedImage

pierlon reviewed Aug 27, 2021

View reviewed changes

assets/src/common/components/pre-publish-panel.js Outdated Show resolved Hide resolved

assets/src/common/components/pre-publish-panel.js Outdated Show resolved Hide resolved

assets/src/common/components/pre-publish-panel.js Outdated Show resolved Hide resolved

pierlon and others added 3 commits August 27, 2021 01:19

Update comments

a3f904a

Merge branch 'develop' into add/post-featured-image-in-pre-publish-panel

9761650

Show a warning notice when there are validation errors for the featur…

ac183ea

…ed image

pierlon requested a review from westonruter August 27, 2021 05:32

Remove unnecessary constant

09e81c8

pierlon approved these changes Aug 27, 2021

View reviewed changes

Place import in correct position

1fb801e

pierlon requested a review from delawski August 27, 2021 15:00

westonruter reviewed Aug 27, 2021

View reviewed changes

delawski approved these changes Aug 30, 2021

View reviewed changes

westonruter merged commit 96e76b1 into ampproject:develop Aug 30, 2021

pierlon mentioned this pull request Aug 30, 2021

[2.1] Add PostFeaturedImage to PrePublishPanel #6580

Merged

2 tasks

westonruter self-assigned this Aug 30, 2021

arthur791004 deleted the add/post-featured-image-in-pre-publish-panel branch August 31, 2021 02:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PostFeaturedImage to PrePublishPanel #6563

Add PostFeaturedImage to PrePublishPanel #6563

arthur791004 commented Aug 25, 2021 •

edited

Loading

CLAassistant commented Aug 25, 2021 •

edited

Loading

pierlon commented Aug 26, 2021

pierlon Aug 26, 2021

arthur791004 Aug 26, 2021

arthur791004 Aug 26, 2021

arthur791004 commented Aug 26, 2021

pierlon Aug 26, 2021

arthur791004 Aug 27, 2021 •

edited

Loading

pierlon Aug 26, 2021

pierlon commented Aug 26, 2021

pierlon commented Aug 27, 2021

codecov bot commented Aug 27, 2021 •

edited

Loading

pierlon left a comment

arthur791004 commented Aug 27, 2021

westonruter Aug 27, 2021

westonruter Aug 27, 2021

westonruter Aug 27, 2021

pierlon Aug 27, 2021 •

edited

Loading

westonruter Aug 27, 2021

pierlon Aug 27, 2021

delawski left a comment •

edited

Loading

pierlon commented Aug 30, 2021

westonruter commented Aug 30, 2021

pierlon commented Aug 30, 2021

westonruter commented Aug 30, 2021

	<PostFeaturedImage noticeUI={ noticeUI } />
	<PostFeaturedImage />

Add PostFeaturedImage to PrePublishPanel #6563

Add PostFeaturedImage to PrePublishPanel #6563

Conversation

arthur791004 commented Aug 25, 2021 • edited Loading

Summary

Screenshots

Checklist

CLAassistant commented Aug 25, 2021 • edited Loading

pierlon commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthur791004 commented Aug 26, 2021

Choose a reason for hiding this comment

arthur791004 Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon commented Aug 26, 2021

pierlon commented Aug 27, 2021

codecov bot commented Aug 27, 2021 • edited Loading

Codecov Report

pierlon left a comment

Choose a reason for hiding this comment

arthur791004 commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delawski left a comment • edited Loading

Choose a reason for hiding this comment

pierlon commented Aug 30, 2021

westonruter commented Aug 30, 2021

pierlon commented Aug 30, 2021

westonruter commented Aug 30, 2021

arthur791004 commented Aug 25, 2021 •

edited

Loading

CLAassistant commented Aug 25, 2021 •

edited

Loading

arthur791004 Aug 27, 2021 •

edited

Loading

codecov bot commented Aug 27, 2021 •

edited

Loading

pierlon Aug 27, 2021 •

edited

Loading

delawski left a comment •

edited

Loading