WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 6 months ago

Last modified 4 months ago

#25325 closed enhancement (fixed)

Twenty Fourteen: Allow pages to have featured images

Reported by: iamtakashi Owned by: lancewillett
Milestone: 3.8 Priority: normal
Severity: normal Version: 3.8
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description

Currently, featured images are limited to posts in Twenty Fourteen. It maybe a good idea to allow pages also to have featured images.

What I'm not 100% sure about is whether we should display the placeholders like posts in case there is no featured image or not.

Attachments (10)

25325.diff (2.7 KB) - added by iamtakashi 7 months ago.
First pass for allowing pages to have featured images.
25325.1.diff (8.1 KB) - added by iamtakashi 7 months ago.
Updated patch
25325.2.diff (16.6 KB) - added by iamtakashi 6 months ago.
25325.3.diff (14.9 KB) - added by iamtakashi 6 months ago.
25325.4.diff (15.4 KB) - added by iamtakashi 6 months ago.
25325.5.diff (2.3 KB) - added by lancewillett 6 months ago.
Rework logic to show link or div correctly
25325.6.diff (5.5 KB) - added by lancewillett 6 months ago.
Better logic for HTML output and add fallback screen-reader-text message
25325.7.diff (9.6 KB) - added by obenland 6 months ago.
Optimal logic for HTML output and template tag rename.
25325.7.2.diff (9.8 KB) - added by lancewillett 6 months ago.
Merged 6 and 7 changes
25325.8.diff (9.8 KB) - added by lancewillett 6 months ago.

Download all attachments as: .zip

Change History (40)

iamtakashi7 months ago

First pass for allowing pages to have featured images.

comment:1 iamtakashi7 months ago

  • Keywords has-patch needs-testing added

comment:2 obenland7 months ago

  • Milestone changed from Awaiting Review to 3.8
  • Version set to trunk

comment:3 follow-ups: Frank Klein7 months ago

Here is my feedback concerning the patch:

  • The patch doesn't apply properly, since it's made against src/wp-content.
  • Does the featured image need a link wrapped around it? On a page, it only links to itself, which seems redundant. The same applies to single posts by the way.
  • If you set a featured image and use the full-width template, it looks kind of broken (image to short, header not overlapping,.... This is related to/dependent on #25031. I think we might need to check if there is a featured image set on the full width page and then apply the corresponding style (via .has-featured-image or something similar).

comment:4 in reply to: ↑ 3 ; follow-up: SergeyBiryukov7 months ago

Replying to Frank Klein:

The patch doesn't apply properly, since it's made against src/wp-content.

That's the path in the new repository, it's used in the majority of patches now.

comment:5 in reply to: ↑ 4 ; follow-up: iamtakashi7 months ago

Replying to SergeyBiryukov:

That's the path in the new repository, it's used in the majority of patches now.

I've just re-uploaded my recent patches in other tickets so that they can be applied from the WordPress root because, traditionally, that's been considered as the best practice as opposed to what I did initially. Would you say the best practice is changing since we have the new repo?

comment:6 in reply to: ↑ 3 iamtakashi7 months ago

Replying to Frank Klein:

  • Does the featured image need a link wrapped around it? On a page, it only links to itself, which seems redundant. The same applies to single posts by the way.

We can definitely remove the link wrapper for pages and use a div instead but maybe it's a good idea to remove the link wrapper for both pages and posts altogether? I've just realised featured images are not linked in both Twenty Twelve and Twenty Thirteen.

Many magazine sites in wild usually link a lead image to its single view though. Of course, we can alter the markup with a conditional tag but I'm not sure if it's worth to do for the sake of simplicity.

  • If you set a featured image and use the full-width template, it looks kind of broken (image to short, header not overlapping,.... This is related to/dependent on #25031. I think we might need to check if there is a featured image set on the full width page and then apply the corresponding style (via .has-featured-image or something similar).

I'll revise the full-width treatment when #25332 progresses.

Another thing I'd like to hear opinions is that the theme should display the placeholder if there is no featured image on a page. I'm slightly leaning towards to not to display at the moment actually. What do you guys think?


comment:7 in reply to: ↑ 5 ; follow-up: SergeyBiryukov7 months ago

Replying to iamtakashi:

I've just re-uploaded my recent patches in other tickets so that they can be applied from the WordPress root because, traditionally, that's been considered as the best practice as opposed to what I did initially. Would you say the best practice is changing since we have the new repo?

Either way is fine. WP root is probably better to make sure the patches can also be applied by those who don't have the whole trunk directory of the new repo checked out.

comment:8 in reply to: ↑ 7 iamtakashi7 months ago

Replying to SergeyBiryukov:

Either way is fine. WP root is probably better to make sure the patches can also be applied by those who don't have the whole trunk directory of the new repo checked out.

OK, thank you!

comment:9 iamtakashi7 months ago

Replying to iamtakashi:

We can definitely remove the link wrapper for pages and use a div instead but maybe it's a good idea to remove the link wrapper for both pages and posts altogether? I've just realised featured images are not linked in both Twenty Twelve and Twenty Thirteen.

Many magazine sites in wild usually link a lead image to its single view though. Of course, we can alter the markup with a conditional tag but I'm not sure if it's worth to do for the sake of simplicity.


Discussed in Twenty Fourteen office hours today. We've decided to link featured images to single on archive views. IRC log

Another thing I'd like to hear opinions is that the theme should display the placeholder if there is no featured image on a page. I'm slightly leaning towards to not to display at the moment actually. What do you guys think?


We've decided to not to show the placeholder for pages in the office hours today. IRC log.

iamtakashi7 months ago

Updated patch

comment:10 lancewillett7 months ago

  • Keywords needs-refresh added; needs-testing removed

Discussed in Tue September 24, 2013 office hours and decide Takashi is going to take another pass at the patch to simplify the logic in content.php if possible.

Instead of all the if/else checks for has_post_thumbnail() using the same markup for all cases and using CSS to adjust the layout.

iamtakashi6 months ago

comment:11 iamtakashi6 months ago

Here is a new patch.

  • Simplify the logic in the template and control the display by CSS.
  • Display featured images on other formats when they are attached.
  • Add a custom post class has-featured-image when it's appropriate.
  • Style adjustments.

comment:12 obenland6 months ago

I wonder if it would make sense to move the post thumbnail logic into its own partial.

Also:

  • Do we keep the title attribute in the post thumbnail link?
  • In the function comment to twentyfourteen_post_classes() it says "Adds body classes to denote:"

iamtakashi6 months ago

comment:13 iamtakashi6 months ago

@obenland, thanks for taking a look. I've updated the patch. Suggested approach is certainly cleaner on each template.

comment:14 lancewillett6 months ago

Patch is looking great. Two suggested changes:

  • Use is_singular() to check for a single post or page.
  • Move the password checks into the function so the templates just have a one-line call.

iamtakashi6 months ago

comment:15 lancewillett6 months ago

  • Owner set to lancewillett
  • Resolution set to fixed
  • Status changed from new to closed

In 25735:

Twenty Fourteen: allow pages to have featured images, props iamtakashi. Fixes #25325.

comment:16 lancewillett6 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

lancewillett6 months ago

Rework logic to show link or div correctly

comment:17 follow-up: lancewillett6 months ago

I noticed in testing that this wasn't working quite right -- the link should appear only on index, and not in single. And we shouldn't check for has_post_thumbnail() at all, right? Otherwise the background pattern won't ever show up.

Also noting that in the a11y ticket, see http://core.trac.wordpress.org/ticket/25054#comment:31 -- this came up as a possible problem for screen readers if the div or anchor element is empty. Could we add a fallback text or something for when no image exists, and hide it with .screen-reader-text maybe?

comment:18 in reply to: ↑ 17 ; follow-up: iamtakashi6 months ago

Replying to lancewillett:

I noticed in testing that this wasn't working quite right -- the link should appear only on index, and not in single. And we shouldn't check for has_post_thumbnail() at all, right? Otherwise the background pattern won't ever show up.

Good catch. You're right. that's also why the placeholder was clickable in in single.

Also noting that in the a11y ticket, see http://core.trac.wordpress.org/ticket/25054#comment:31 -- this came up as a possible problem for screen readers if the div or anchor element is empty. Could we add a fallback text or something for when no image exists, and hide it with .screen-reader-text maybe?

OK, but what would be the appropriate text? Something like "No Featured Image"?

lancewillett6 months ago

Better logic for HTML output and add fallback screen-reader-text message

comment:19 lancewillett6 months ago

  • Keywords needs-refresh removed

Refreshed patch attached.

obenland6 months ago

Optimal logic for HTML output and template tag rename.

comment:20 in reply to: ↑ 18 ; follow-up: obenland6 months ago

Replying to lancewillett:

Also noting that in the a11y ticket, see http://core.trac.wordpress.org/ticket/25054#comment:31 -- this came up as a possible problem for screen readers if the div or anchor element is empty. Could we add a fallback text or something for when no image exists, and hide it with .screen-reader-text maybe?

Is it really an a11y issue? Or is he just surprised to see an empty element there?

comment:21 in reply to: ↑ 20 ; follow-up: lancewillett6 months ago

Replying to obenland:

Replying to lancewillett:
Is it really an a11y issue? Or is he just surprised to see an empty element there?

From my understanding screen readers speak aloud the link text so the user knows what the end result would be for following the link. It's also just bad practice to have an empty HTML element, anyway.

comment:22 in reply to: ↑ 21 ; follow-up: obenland6 months ago

Replying to lancewillett:

From my understanding screen readers speak aloud the link text so the user knows what the end result would be for following the link. It's also just bad practice to have an empty HTML element, anyway.

I understand. The problem with though is that we'd have to have a link in a link, to both have it styled with the background image and explained with a screenreader text. If we'd just add text in the empty post thumbnail link it would not be focusable.

comment:23 in reply to: ↑ 22 ; follow-up: lancewillett6 months ago

Replying to obenland:

Replying to lancewillett:
The problem with though is that we'd have to have a link in a link, to both have it styled with the background image and explained with a screenreader text. If we'd just add text in the empty post thumbnail link it would not be focusable.

With changes in my latest patch, it should work perfectly. Now to test it more. :)

The patch adds the placeholder text so that the div or a is never empty.

comment:24 in reply to: ↑ 23 obenland6 months ago

Replying to lancewillett:

With changes in my latest patch, it should work perfectly. Now to test it more. :)
The patch adds the placeholder text so that the div or a is never empty.

Are paragraph tags focusable?

Last edited 6 months ago by obenland (previous) (diff)

lancewillett6 months ago

Merged 6 and 7 changes

lancewillett6 months ago

comment:25 lancewillett6 months ago

.8 is my proposed solution, but I'm open to discussing the pros and cons of leaving the HTML element empty versus adding the placeholder text.

comment:26 lancewillett6 months ago

Asked @joedolson for more feedback on .8 proposed patch (see #25054).

comment:27 follow-up: joedolson6 months ago

Hi, Lance - sorry about the delay in getting back to you.

  • What effect does an empty element have on a11y?
  • If the element has text, but isn't focusable, is that a problem?

An empty element that does not perform an action (such as a div or paragraph) has no impact on accessibility; it will be ignored, as long it doesn't take focus.

If an element that performs an action isn't focusable, that's a problem, but if it doesn't do anything, then that doesn't matter. The important thing is that any actionable element -- anytime you need to activate an element to accomplish a task -- then that element must have text, and must be focusable.

A link with no text will generally be read with the href value, which will possible to understand, when read character-by-character is likely to be fairly useless.

comment:28 in reply to: ↑ 27 lancewillett6 months ago

Replying to joedolson:

Thanks, Joe. :)

comment:29 lancewillett6 months ago

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

In 25802:

Twenty Fourteen: better logic for featured image HTML output, and add fallback message for focusable anchor elements, for accessibility. Fixes #25325.

comment:30 lancewillett4 months ago

In 26694:

Twenty Fourteen: remove invalid rel attributes from post thumbnail output, and fix a CSS typo. See #25946 and #25325.

Note: See TracTickets for help on using tickets.