WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 13 months ago

Last modified 13 months ago

#23620 closed defect (bug) (fixed)

Twenty Thirteen: extract video or image from post content and place above the title and post content

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

Description

Depends on new video and image grabber functions in #23572.

This is to meet design goals mocked up by @joen:

  • Let video and image content be 724 pixels wide
  • Keep text in video and image post formats at 604 pixels wide to match post meta width
  • Video and image content should come *before* anything else. Then the title, followed by any other textual content.

Note: Twenty Thirteen does not need back compat with pre 3.6 versions.

Attachments (9)

23620.diff (2.3 KB) - added by lancewillett 13 months ago.
23620.2.diff (2.5 KB) - added by lancewillett 13 months ago.
23620.3.diff (3.6 KB) - added by lancewillett 13 months ago.
23620.4.diff (6.2 KB) - added by lancewillett 13 months ago.
23620.5.diff (1.9 KB) - added by lancewillett 13 months ago.
23620.6.diff (2.5 KB) - added by lancewillett 13 months ago.
23620.7.diff (2.8 KB) - added by lancewillett 13 months ago.
Uses the_extra_content() instead of the_content() for video and image templates
23620.8.diff (4.1 KB) - added by obenland 13 months ago.
23620.9.diff (4.1 KB) - added by obenland 13 months ago.

Download all attachments as: .zip

Change History (41)

comment:1 sabreuse14 months ago

  • Cc sabreuse added

comment:2 lancewillett14 months ago

  • Keywords twentythirteen added

comment:3 lancewillett14 months ago

  • Keywords twentythirteen removed

comment:4 lancewillett13 months ago

#23542 was marked as a duplicate.

comment:5 lancewillett13 months ago

In 23786:

Twenty Thirteen: we loves us some structured post formats. See #23619 #23620 #23621.

lancewillett13 months ago

comment:6 lancewillett13 months ago

  • Keywords has-patch added; needs-patch removed

23620.diff is a proof of concept, first pass -- for video only. One thing I can't get to work yet is removing the video from the post content.

Testing with a YouTube URL as the first line in the post content, then a few lines of text below it.

comment:7 lancewillett13 months ago

If we go with something like this, we'll probably want to abstract to both video/audio and use the same logic both places.

comment:8 obenland13 months ago

If you run the video through the_content(), you'll end up with twice of whatever is hooked in there to append their after-content junk goodness.

lancewillett13 months ago

comment:9 lancewillett13 months ago

Second pass: returns array to content-video.php template with 0 being bare URL of the video, 1 being the rest of the content without the video.

comment:10 follow-up: lancewillett13 months ago

I still can't get the filtered content without the media URL bit when using the_content() in the template file, it outputs the full content.

comment:11 in reply to: ↑ 10 obenland13 months ago

Replying to lancewillett:

I still can't get the filtered content without the media URL bit when using the_content() in the template file, it outputs the full content.

For that to happen you probably would have to update $post->post_content in the $post global.

lancewillett13 months ago

comment:12 lancewillett13 months ago

Much better working example in .3 patch.

Remove logic from template, just call functions there. Add check for structured post meta.

lancewillett13 months ago

comment:13 lancewillett13 months ago

.4 patch adds first pass at image post format support.

Two big issues:

  1. Post format "add image to post" functionality in wp-admin doesn't currently allow filtering for 724 pixel width instead of the default $content_width of 604 pixels. See #23863

It could be a moot point if we build our own HTML output (but I'd rather not do that, if possible).

  1. We need to decide on building the image HTML output by hand based on a returned image "src" value -- or a better way to get the parsed images back. I proposed two options here: http://core.trac.wordpress.org/ticket/23572#comment:13 -- 1) get back full image HTML instead of just the source URL and 2) get back attachment IDs if they exist so we can run them through wp_get_attachment_image() to get the exactly sized image we'd like, at 724 pixels width.

Fixing both of these issues would avoid us resizing images down with CSS scaling. With .4 patch above we do just that: load full-size image into HTML source, use max-width: 724px; to scale it down.

lancewillett13 months ago

comment:14 lancewillett13 months ago

Updated patch to work with latest patch on #23572: http://core.trac.wordpress.org/attachment/ticket/23572/23572-html.diff

Note, image post format isn't handled here, yet. It isn't a structured post format (no post meta saved).

comment:15 follow-up: obenland13 months ago

I think we should give the_video() and the_image() their own div above the header. class="entry-media" or something?

comment:16 in reply to: ↑ 15 lancewillett13 months ago

Replying to obenland:

I think we should give the_video() and the_image() their own div above the header. class="entry-media" or something?

+1 to <div class="entry-media"> for both video and image. I'll add it to my next patch.

lancewillett13 months ago

comment:17 lancewillett13 months ago

.6 adds HTML div wrappers for both video and image.

comment:18 lancewillett13 months ago

In 23803:

Twenty Thirteen: explicitly declare 'link' and 'video' as the only (current) structured post formats in Twenty Thirteen. See #23852 #23619 #23864 and #23620.

lancewillett13 months ago

Uses the_extra_content() instead of the_content() for video and image templates

comment:19 lancewillett13 months ago

New patch for Twenty Thirteen, using the_extra_content() instead of the_content() for video and image templates -- goes with attachment:ticket:23572:23572-html.9.diff changes.

comment:20 lancewillett13 months ago

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

In 23820:

Twenty Thirteen: take full advantage of new structured post format template functions, see r23819.

Extract first video or image from Video and Image post format posts, and place the item above the title within its own HTML wrapper with the_video() and the_image()'. Also, remove the item from the post content with the_extra_content()`.

This allows us to meet the design requirements for Twenty Thirteen with minimal changes to the theme template files. Fixes #23620.

comment:21 obenland13 months ago

  • Keywords has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:22 Mamaduka13 months ago

Looks good.

Still need to figure out how to filter $content_width to display videos properly on archive pages.

comment:23 lancewillett13 months ago

@Mamaduka Can you post screenshot, example WP URL structure, or test URLs to look at? So we can repeat and fix.

comment:24 obenland13 months ago

/type/video works for me

obenland13 months ago

comment:25 obenland13 months ago

  • Keywords has-patch added

comment:26 follow-up: lancewillett13 months ago

Nice patch! Can you explain it a bit?

  • This seems dangerous .entry-media > *
  • Why did you remove wp-caption styles?
  • Why merge anchor and anchor hover colors?
  • Why is the has_post_format( 'image' ) no longer needed?

comment:27 Mamaduka13 months ago

My bad, it was caused by caching, after updating post everything looks right.

comment:28 in reply to: ↑ 26 obenland13 months ago

Replying to lancewillett:

  • This seems dangerous .entry-media > *

We use .entry-media pretty selectively and the selector only targets elements that are direct descendants. I was just not sure how verbose we'd need to be, meaning how many elements we would have to account for.


  • Why did you remove wp-caption styles?

Since we single out the featured image and it won't have any captions, they are no longer necessary. Everything within .format-image .entry-content will get treated like regular content, including default caption styles.


  • Why merge anchor and anchor hover colors?

The hover orange won't work with a11y. The merge would change the hover effect to just the underline. maybe this could be something Joen looks at in his final review?


  • Why is the has_post_format( 'image' ) no longer needed?

Because we use a custom image size.

comment:29 lancewillett13 months ago

Thanks for the notes. Should we name "twentythirteen-featured-image" something more like "twentythirteen-image-post-format" or something more related to what it's used for?

Should we constrain the height by something more reasonable like 965 pixels (gives a 724 wide image in that case)?

comment:30 obenland13 months ago

Yeah, we can do both. Though we might want to account for 16:9 portrait images. 1288px?

obenland13 months ago

comment:31 lancewillett13 months ago

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

In 23839:

Twenty Thirteen: improvements to Image post format handling to use a custom image size instead of filtering a $content_width variable.

Includes CSS cleanup and markup improvements to both video and image templates, moving the media HTML piece out of the header element.

Props obenland, closes #23620.

comment:32 lancewillett13 months ago

In 23848:

Twenty Thirteen: simpler name for custom image size for image post formats, see #23620.

Note: See TracTickets for help on using tickets.