Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years 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's profile lancewillett Owned by: lancewillett's profile 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 11 years ago.
23620.2.diff (2.5 KB) - added by lancewillett 11 years ago.
23620.3.diff (3.6 KB) - added by lancewillett 11 years ago.
23620.4.diff (6.2 KB) - added by lancewillett 11 years ago.
23620.5.diff (1.9 KB) - added by lancewillett 11 years ago.
23620.6.diff (2.5 KB) - added by lancewillett 11 years ago.
23620.7.diff (2.8 KB) - added by lancewillett 11 years ago.
Uses the_extra_content() instead of the_content() for video and image templates
23620.8.diff (4.1 KB) - added by obenland 11 years ago.
23620.9.diff (4.1 KB) - added by obenland 11 years ago.

Download all attachments as: .zip

Change History (41)

#1 @sabreuse
11 years ago

  • Cc sabreuse added

#2 @lancewillett
11 years ago

  • Keywords twentythirteen added

#3 @lancewillett
11 years ago

  • Keywords twentythirteen removed

#4 @lancewillett
11 years ago

#23542 was marked as a duplicate.

#5 @lancewillett
11 years ago

In 23786:

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

@lancewillett
11 years ago

#6 @lancewillett
11 years 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.

#7 @lancewillett
11 years 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.

#8 @obenland
11 years 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.

#9 @lancewillett
11 years 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.

#10 follow-up: @lancewillett
11 years 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.

#11 in reply to: ↑ 10 @obenland
11 years 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.

#12 @lancewillett
11 years ago

Much better working example in .3 patch.

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

#13 @lancewillett
11 years 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.

#14 @lancewillett
11 years 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).

#15 follow-up: @obenland
11 years ago

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

#16 in reply to: ↑ 15 @lancewillett
11 years 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.

#17 @lancewillett
11 years ago

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

#18 @lancewillett
11 years 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.

@lancewillett
11 years ago

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

#19 @lancewillett
11 years 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.

#20 @lancewillett
11 years 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.

#21 @obenland
11 years ago

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

#22 @Mamaduka
11 years ago

Looks good.

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

#23 @lancewillett
11 years ago

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

#24 @obenland
11 years ago

/type/video works for me

@obenland
11 years ago

#25 @obenland
11 years ago

  • Keywords has-patch added

#26 follow-up: @lancewillett
11 years 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?

#27 @Mamaduka
11 years ago

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

#28 in reply to: ↑ 26 @obenland
11 years 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.

#29 @lancewillett
11 years 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)?

#30 @obenland
11 years ago

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

@obenland
11 years ago

#31 @lancewillett
11 years 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.

#32 @lancewillett
11 years 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.