Make WordPress Core

Opened 7 years ago

Closed 28 hours ago

#39281 closed enhancement (fixed)

Twenty Seventeen: header.php forces thumbnails on all post types

Reported by: justnorris's profile justnorris Owned by: poena's profile poena
Milestone: 6.6 Priority: normal
Severity: normal Version: 4.8
Component: Bundled Theme Keywords: has-patch needs-testing has-testing-info
Focuses: ui, template Cc:

Description

My plugin has custom post type and custom 'single-post-type' views and doesn't utilize thumbnails in them. As a result layout appears broken, and there is nothing I can do to make the plugin compatible with Twentyseventeen.

I went to have a look how WooCommerce deals with this, and it turns out that there is this piece of code in the style of twentyseventeen:

.single-product .single-featured-image-header {
    display: none
}

While that does work, I don't think it's a solution. At the very least there has to be a filter to dynamically disable the thumbnail from header.php

Attachments (9)

39281.diff (1.7 KB) - added by JPry 7 years ago.
100p.png (1.0 MB) - added by justnorris 6 years ago.
100% zoom
50p.png (917.2 KB) - added by justnorris 6 years ago.
50% Zoom
WordPress-Develop.png (2.6 MB) - added by lamarajan 8 days ago.
Home page
Test-Page-–-WordPress-Develop.png (2.5 MB) - added by lamarajan 8 days ago.
Sample page after applying patche/
Test-Post-–-WordPress-Develop.png (2.6 MB) - added by lamarajan 8 days ago.
Sample Post after applying patch.
Test-Page-–-WordPress-Develop with apply filter.png (373.1 KB) - added by lamarajan 8 days ago.
Sample page after applying patch and enable filter.
Test-Post-–-WordPress-Develop-with applyfilter.png (463.6 KB) - added by lamarajan 8 days ago.
Sample post after applying patch and enable filter.
WordPress-Develop-homepage.png (2.6 MB) - added by lamarajan 8 days ago.
Sample home page after applying patch and enable filter.

Change History (36)

#1 @JPry
7 years ago

  • Focuses performance removed
  • Resolution set to worksforme
  • Status changed from new to closed

Hey @justnorris,

Thanks for taking the time to log a ticket here on Trac!

The twentyseventeen theme will only display a thumbnail when has_post_thumbnail() is true. As long there's no featured image set, then the theme won't try to display one. That means that there's no need to disable the thumbnail. Here's the relevant section of header.php:

        <?php
        // If a regular post or page, and not the front page, show the featured image.
        if ( has_post_thumbnail() && ( is_single() || ( is_page() && ! twentyseventeen_is_frontpage() ) ) ) :
                echo '<div class="single-featured-image-header">';
                the_post_thumbnail( 'twentyseventeen-featured-image' );
                echo '</div><!-- .single-featured-image-header -->';
        endif;
        ?>

It appears that WooCommerce is simply taking an extra precaution with hiding a possible featured image.

When registering your custom post type, you can also make sure that it does not support thumbnail in the supports parameter. This will ensure that users cannot even set a featured image to begin with.

#2 @justnorris
7 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Hey @JPry, thanks for the response. However, I have to disagree. My custom-post-type does support thumbnails, but I utilize them in a different way. Because of the current state of twentyseventeen I can't make the plugin compatible with the default WordPress theme. I think every WordPress plugin should look and feel great out of the box, with best practices intact.

For my Photography Portfolio plugin - featured images are used in archives, but once a gallery is opened - a different set of images are used. I don't need to show the featured-image there, so at the moment, I have to resort to a hack like this one: https://github.com/justnorris/photography-portfolio/commit/d85b14b0232830fbceeb95eac10e5b196d10ae35

I'd really appreciate it if at least a filter was introduced. I can't be the only person on earth doing a custom view for a custom post type.

In any case - I think the header.php readability will too benefit from something like this:

<?php
        // If a regular post or page, and not the front page, show the featured image.
        if ( twentyseventeen_should_show_featured_image() ) :
                echo '<div class="single-featured-image-header">';
                the_post_thumbnail( 'twentyseventeen-featured-image' );
                echo '</div><!-- .single-featured-image-header -->';
        endif;

        // functions.php
        function twentyseventeen_should_show_featured_image() {
                return apply_filters( 'twentyseventeen_should_show_featured_image', ( has_post_thumbnail() && ( is_single() || ( is_page() && ! twentyseventeen_is_frontpage() ) ) ) );
        }
        

        ?>

Normally things like thumbnails belong to single.php, archive.php, etc., Twenty Seventeen does things a bit differently, that's alright, as long as it doesn't break things for others.

Version 0, edited 7 years ago by justnorris (next)

@JPry
7 years ago

#3 @JPry
7 years ago

  • Keywords has-patch added
  • Type changed from defect (bug) to enhancement

Replying to justnorris:

My custom-post-type does support thumbnails, but I utilize them in a different way.

That's different than what you said originally, which is why my original suggestion was to ensure your plugin wasn't providing support for thumbnails.

Having a filter certainly seems feasible to me.

#4 @justnorris
7 years ago

Sorry for not being clear the first time around. Thanks for the patch!

#5 @davidakennedy
7 years ago

  • Keywords reporter-feedback added

Hey @justnorris! Thanks for the report!

As a result layout appears broken, and there is nothing I can do to make the plugin compatible with Twentyseventeen.

Can you add some screenshots so I can see how it's broken? It would help determine the best path forward.

Also, thanks @JPry for the initial patch.

#6 @SergeyBiryukov
7 years ago

  • Summary changed from [twentyseventeen] header.php forces thumbnails on all post types to Twenty Seventeen: header.php forces thumbnails on all post types

@justnorris
6 years ago

100% zoom

@justnorris
6 years ago

50% Zoom

#7 @justnorris
6 years ago

@davidakennedy Ha, I didn't get a notification about the issue, and unfortunately the issue is still present 12 months later, even with the patch 😔

Here are the screenshots, I had to zoom out to 50% just to capture the general idea of the layout. It's a view of a single portfolio entry with a description and a gallery. The featured image is really unnecessary there.

#8 @justnorris
6 years ago

  • Keywords reporter-feedback removed

#9 @zkarj
6 years ago

Having just found this issue myself, I concur with the OP that header.php is not a logical place for the featured image unless that image is visually integrated into the header – e.g. replacing the header image. This does not appear to be the case with Twenty Seventeen.

In my case, I am developing a personal child theme so was able to override header.php and comment out the relevant block. It took me a while to figure out where the image was coming from!

#11 @poena
3 months ago

Testing instructions

Apply PR https://github.com/WordPress/wordpress-develop/pull/6157.
Activate the theme
Create two pages and a post with featured images.
View the pages and post on the front.

Expected Results

When testing a patch to validate it works as expected:

  • ✅ On the single post and the single page, the featured image shows.

Assign one of the pages as the front page:
Go to the WordPress admin, Settings, Reading, and change
"Your homepage displays" to a static page, and select the page you just created.

Expected Results

When testing a patch to validate it works as expected:

  • ✅ The theme header image and the page featured image shows once.

To test the filter, add the following in functions.php after twentyseventeen_should_show_featured_image(), or in a child theme or plugin:

add_filter( 'twentyseventeen_should_show_featured_image', '__return_false' );

Expected Results

When testing a patch to validate it works as expected:

  • ✅ On the static front page with the page assigned, the theme header image and the page featured image should show once.
  • ✅ On the single post and the single page, the featured image should not show.
Last edited 3 months ago by poena (previous) (diff)

#13 @poena
3 months ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 6.6

#14 @poena
3 months ago

The reason why the featured image still shows on the static front page is because of the themes "panel image" feature.
Without the condition in twentyseventeen_should_show_featured_image(), the panels are broken.

This ticket was mentioned in Slack in #core-themes by karmatosed. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core by nhrrob. View the logs.


9 days ago

#17 @nhrrob
9 days ago

  • Keywords has-testing-info added

This one has testing instructions and needs testing.
Updating keywords accordingly.

This ticket was mentioned in Slack in #core-test by nhrrob. View the logs.


9 days ago

#19 @lamarajan
8 days ago

Hello @poena and @nhrrob,

I have tested as per given instructions with PR.

Expected Results
When testing a patch to validate it works as expected:
✅ On the single post and the single page, the featured image shows.
Assign one of the pages as the front page:
Go to the WordPress admin, Settings, Reading, and change
"Your homepage displays" to a static page, and select the page you just created.

PASS

Expected Results
When testing a patch to validate it works as expected:
✅ The theme header image and the page featured image shows once.
To test the filter, add the following in functions.php after twentyseventeen_should_show_featured_image(), or in a child theme or plugin:
add_filter( 'twentyseventeen_should_show_featured_image', 'return_false' );

PASS

Expected Results
When testing a patch to validate it works as expected:
✅ On the static front page with the page assigned, the theme header image and the page featured image should show once.

PASS

✅ On the single post and the single page, the featured image should not show.
It is working as aspected.

PASS

I have also attached the screenshot.

Thanks,
Rajan Lama (@lamarajan)

@lamarajan
8 days ago

Home page

@lamarajan
8 days ago

Sample page after applying patche/

@lamarajan
8 days ago

Sample Post after applying patch.

@lamarajan
8 days ago

Sample page after applying patch and enable filter.

@lamarajan
8 days ago

Sample post after applying patch and enable filter.

@lamarajan
8 days ago

Sample home page after applying patch and enable filter.

This ticket was mentioned in Slack in #core-test by lamarajan. View the logs.


8 days ago

This ticket was mentioned in Slack in #core-test by lamarajan. View the logs.


8 days ago

#22 @karmatosed
5 days ago

  • Keywords commit added
  • Owner set to karmatosed
  • Status changed from reopened to assigned

It looks like this is tested and ready to move to commit which is great. I will add that keyword and look to move it there. Thanks everyone.

#23 @karmatosed
3 days ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 58206:

Twenty Seventeen: Add filter for display of featured image in header.

When a layout doesn't use custom post types it was appearing broken. This adds a filter for the display of the featured image to resolve this.

Props justnorris, JPry, davidakennedy, SergeyBiryukov, zkarj, poena, nhrrob, lamarajan.
Fixes #39281.

#24 @karmatosed
3 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This is being reopened to get the documentation also in and assigned to @poena to see that through.

#25 @karmatosed
3 days ago

  • Owner changed from karmatosed to poena
  • Status changed from reopened to assigned

#26 @karmatosed
3 days ago

  • Keywords commit removed

This ticket was mentioned in PR #6657 on WordPress/wordpress-develop by @poena.


3 days ago
#27

…tured_image

Trac ticket: https://core.trac.wordpress.org/ticket/39281

The new function `twentyseventeen_should_show_featured_image is missing documentation for the custom filter with the same name.

This PR updates the summary and adds an example of how to use the filter.

#28 @karmatosed
28 hours ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 58237:

Twenty Seventeen: Updates docblock for function.

The new function twentyseventeen_should_show_featured_image was missing documentation for the custom filter. This brings that in.

Props poena.
Fixes #39281.

Note: See TracTickets for help on using tickets.