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 PostFeaturedImage to PrePublishPanel #6563

8 changes: 2 additions & 6 deletions assets/src/block-editor/plugins/pre-publish-panel.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,12 @@
* Internal dependencies
*/
import { PrePublishPanel } from '../../common/components';
import { getMinimumFeaturedImageDimensions } from '../../common/helpers';

export const name = 'amp-post-featured-image-pre-publish-panel';

// On clicking 'Publish,' display a notice if no featured image exists or its width is too small.
// Add the featured image selection component as a pre-publish check.
export const render = () => {
return (
<PrePublishPanel
dimensions={ getMinimumFeaturedImageDimensions() }
required={ false }
/>
<PrePublishPanel />
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,37 @@ import { isFunction } from 'lodash';
*/
import { createHigherOrderComponent } from '@wordpress/compose';
import { Notice } from '@wordpress/components';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { validateFeaturedImage, getMinimumFeaturedImageDimensions } from '../../helpers';

/**
* Create notice UI for featured image component.
*
* @param {string[]} messages Notices.
* @param {string} status Status type of notice.
* @return {JSX.Element} Notice component.
*/
const createNoticeUI = ( messages, status ) => {
return (
<Notice
status={ status }
isDismissible={ false }
>
{ messages.map( ( message, index ) => {
return (
<p key={ `message-${ index }` }>
{ message }
</p>
);
} ) }
</Notice>
);
};

/**
* Higher-order component that is used for filtering the PostFeaturedImage component.
*
Expand All @@ -30,29 +55,18 @@ export default createHigherOrderComponent(

const withFeaturedImageNotice = ( props ) => {
const { media } = props;
let noticeUI;

const errors = validateFeaturedImage( media, getMinimumFeaturedImageDimensions(), false );

if ( ! errors ) {
return <PostFeaturedImage { ...props } />;
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;
}

return (
<>
<Notice
status="warning"
isDismissible={ false }
>
{ errors.map( ( errorMessage, index ) => {
return (
<p key={ `error-${ index }` }>
{ errorMessage }
</p>
);
} ) }
</Notice>
<PostFeaturedImage { ...props } />
</>
<PostFeaturedImage { ...props } noticeUI={ noticeUI } />
);
};

Expand Down
64 changes: 9 additions & 55 deletions assets/src/common/components/pre-publish-panel.js
Original file line number Diff line number Diff line change
@@ -1,73 +1,27 @@
/**
* External dependencies
*/
import PropTypes from 'prop-types';

/**
* WordPress dependencies
*/
import { Notice } from '@wordpress/components';
import { PostFeaturedImage } from '@wordpress/editor';
import { PluginPrePublishPanel } from '@wordpress/edit-post';
import { withSelect } from '@wordpress/data';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { validateFeaturedImage } from '../helpers';

/**
* Conditionally adds a notice to the pre-publish panel for the featured image.
* Adds a pre-publish panel containing the featured image selection component.
*
* @param {Object} props Component props.
* @param {Object} props.featuredMedia Media object.
* @param {Array} props.dimensions Required image dimensions.
* @param {boolean} props.required Whether selecting a featured image is required.
* @return {Function} Either a plain pre-publish panel, or the panel with a featured image notice.
* Note: The `PostFeaturedImage` component would have already been filtered to include
* any notices for the featured image so there is no need to recreate them here.
*
* @return {Function} A pre-publish panel containing the featured image selection component.
*/
const PrePublishPanel = ( { featuredMedia, dimensions, required } ) => {
const errors = validateFeaturedImage( featuredMedia, dimensions, required );

if ( ! errors ) {
return null;
}

const PrePublishPanel = () => {
return (
<PluginPrePublishPanel
title={ __( 'Featured Image', 'amp' ) }
initialOpen="true"
>
<Notice
status={ required ? 'warning' : 'notice' }
isDismissible={ false }
>
{ errors.map( ( errorMessage, index ) => {
return (
<p key={ `error-${ index }` }>
{ errorMessage }
</p>
);
} ) }
</Notice>
<PostFeaturedImage />
</PluginPrePublishPanel>
);
};

PrePublishPanel.propTypes = {
featuredMedia: PropTypes.object,
dimensions: PropTypes.shape( {
width: PropTypes.number.isRequired,
height: PropTypes.number.isRequired,
} ),
required: PropTypes.bool,
};

export default withSelect( ( select ) => {
const currentPost = select( 'core/editor' ).getCurrentPost();
const editedFeaturedMedia = select( 'core/editor' ).getEditedPostAttribute( 'featured_media' );
const featuredMedia = currentPost.featured_media || editedFeaturedMedia;

return {
featuredMedia: featuredMedia ? select( 'core' ).getMedia( featuredMedia ) : null,
};
} )( PrePublishPanel );
export default PrePublishPanel;
25 changes: 10 additions & 15 deletions assets/src/common/helpers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,24 +65,19 @@ export const getMinimumFeaturedImageDimensions = () => {
/**
* Validates the an image based on requirements.
*
* @param {Object} media A media object.
* @param {string} media.mime_type The media item's mime type.
* @param {Object} media.media_details A media details object with width and height values.
* @param {number} media.media_details.width Media width in pixels.
* @param {number} media.media_details.height Media height in pixels.
* @param {Object} dimensions An object with minimum required width and height values.
* @param {number} dimensions.width Minimum required width value.
* @param {number} dimensions.height Minimum required height value.
* @param {boolean} required Whether the image is required or not.
* @param {Object|null} media A media object.
* @param {string} media.mime_type The media item's mime type.
* @param {Object} media.media_details A media details object with width and height values.
* @param {number} media.media_details.width Media width in pixels.
* @param {number} media.media_details.height Media height in pixels.
* @param {Object} dimensions An object with minimum required width and height values.
* @param {number} dimensions.width Minimum required width value.
* @param {number} dimensions.height Minimum required height value.
* @return {string[]|null} Validation errors, or null if there were no errors.
*/
export const validateFeaturedImage = ( media, dimensions, required ) => {
export const validateFeaturedImage = ( media, dimensions ) => {
if ( ! media ) {
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' ) ];
Comment on lines -81 to +80
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@pierlon pierlon Aug 27, 2021

Choose a reason for hiding this comment

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

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;
}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep.

}

const errors = [];
Expand Down
12 changes: 2 additions & 10 deletions assets/src/common/helpers/test/validateFeaturedImage.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,15 @@
import { validateFeaturedImage } from '../';

describe( 'validateFeaturedImage', () => {
it( 'returns an error if `media` is not an object', () => {
const isValid = validateFeaturedImage( null, {}, false );
expect( isValid ).toStrictEqual( [ 'Selecting a featured image is recommended for an optimal user experience.' ] );
} );

it( 'returns an error if the `media` object is not an object and is required', () => {
const isValid = validateFeaturedImage( null, null, true );
it( 'returns an error if the `media` is not an object', () => {
const isValid = validateFeaturedImage( null, { width: 10, height: 10 } );
expect( isValid ).toStrictEqual( [ 'Selecting a featured image is required.' ] );
} );

it( 'returns an error if the featured image is not an acceptable media format', () => {
const isValid = validateFeaturedImage(
{ mime_type: 'foo', media_details: { width: 11, height: 11 } },
{ width: 10, height: 10 },
false,
);
expect( isValid ).toStrictEqual( [ 'The featured image must be of either JPEG, PNG, GIF, WebP, or SVG format.' ] );
} );
Expand All @@ -27,7 +21,6 @@ describe( 'validateFeaturedImage', () => {
const isValid = validateFeaturedImage(
{ mime_type: 'image/png', media_details: { width: 10, height: 10 } },
{ width: 11, height: 11 },
false,
);
expect( isValid ).toStrictEqual( [ 'The featured image should have a size of at least 11 by 11 pixels.' ] );
} );
Expand All @@ -36,7 +29,6 @@ describe( 'validateFeaturedImage', () => {
const isValid = validateFeaturedImage(
{ mime_type: 'image/png', media_details: { width: 11, height: 11 } },
{ width: 10, height: 10 },
false,
);
expect( isValid ).toBeNull();
} );
Expand Down
Loading