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

Only display featured image UI when theme supports it too #6510

Closed
wants to merge 3 commits into from

Conversation

danielbachhuber
Copy link
Member

Description

Ensures the "Featured Image" UI only displays when the theme supports post-thumbnails too.

Fixes #2523

How has this been tested?

Using Twenty Seventeen, here's the UI that normally displays:

image

Then, add the following code snippet to a wp-content/mu-plugins/local.php file:

add_action( 'after_setup_theme', function(){
	remove_theme_support( 'post-thumbnails' );
}, 100 );

The "Featured Image" will no longer display:

image

Types of changes

My code ensures PostTypeSupportCheck also respects the theme's post-thumbnails setting. It introduces post-thumbnails=>true on the site index for exposure through the REST API.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@danielbachhuber danielbachhuber added this to the 2.8 milestone Apr 30, 2018
@danielbachhuber danielbachhuber requested a review from a team April 30, 2018 23:00
supportKeys={ supportKeys }
postType={ postType }
themeSupports={ themeSupports }>foobar</PostTypeSupportCheck> );
expect( wrapper.type() ).not.toBe( null );
Copy link
Member

Choose a reason for hiding this comment

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

Can we use more targeted expectation in here to ensure it properly renders foobar text? One of the ways of doing it is to use toHaveText matcher.

@gziolo
Copy link
Member

gziolo commented May 2, 2018

I haven't tested but the code looks good 👍

);

if ( ! isSupported ) {
return null;
}

// 'thumbnail' and 'post-thumbnails' are intentionally different.
if ( includes( supportKeys, 'thumbnail' ) &&
! get( themeSupports, 'post-thumbnails', false ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

This must be updated to pass the second argument as an array, or will fail master build. (#6247)

);

if ( ! isSupported ) {
return null;
}

// 'thumbnail' and 'post-thumbnails' are intentionally different.
if ( includes( supportKeys, 'thumbnail' ) &&
Copy link
Member

Choose a reason for hiding this comment

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

Why does PostTypeSupportCheck need to be handling this exception? If they're truly intentionally different, why lump them to be considered together? Should it be up to the consuming component to check for both the post type and theme supports? Notably, I see this as being part of the reason PostFeaturedImageCheck exists as a standalone component, so it can become:

function PostFeaturedImageCheck( props ) {
	return (
		<ThemeSupportCheck supportKeys="post-thumbnails">
			<PostTypeSupportCheck { ...props } supportKeys="thumbnail" />
		</ThemeSupportCheck>
	);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

ThemeSupportCheck is a good suggestion. I'll open a new PR with a different implementation.

@danielbachhuber
Copy link
Member Author

Closing in favor of #6541

@danielbachhuber danielbachhuber deleted the 2523-theme-supports-thumbnail branch May 2, 2018 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants