Make WordPress Core

Opened 11 years ago

Closed 10 years ago

#29146 closed defect (bug) (fixed)

Audio/Video in Twenty Thirteen are lacking bottom margin

Reported by: wonderboymusic's profile wonderboymusic Owned by: mikeyarce's profile mikeyarce
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.6
Component: Bundled Theme Keywords: good-first-bug has-patch commit
Focuses: ui Cc:

Description

This is probably a regression, just want to track with a ticket.

Posts that have audio and video: the comment link is too close to the media.

Attachments (10)

thirteen-audio-video.png (174.5 KB) - added by obenland 11 years ago.
Screen Shot 2014-08-07 at 2.42.00 PM.png (279.8 KB) - added by wonderboymusic 11 years ago.
trac-29146.png (343.1 KB) - added by amcreative 11 years ago.
Screenshot with video from WordPress.tv
29146.diff (457 bytes) - added by mikeyarce 11 years ago.
fix bottom margin for video, videopress, and audio embeds
29146.1.diff (568 bytes) - added by mikeyarce 11 years ago.
updated path
29146.2.diff (397 bytes) - added by jeanyoungkim 10 years ago.
29146.3.diff (652 bytes) - added by ccprice 10 years ago.
29146.4.diff (708 bytes) - added by DavidTheMachine 10 years ago.
change filepaths so patch will apply
29146.5.diff (1.5 KB) - added by mikeyarce 10 years ago.
Fixes bottom margin in Twenty Ten, Twenty Eleven, and Twenty Thirteen for the Video Player, VideoPress, and Audio Player elements in the Standard Post Format. Tested all default themes up to Twenty Fifteen - all other themes either have top margin in the footer .entry-meta or aren't affected.
29146.6.diff (1.8 KB) - added by gregrickaby 10 years ago.
Includes all fixes in .5, includes support for .format-audio

Download all attachments as: .zip

Change History (34)

#1 @obenland
11 years ago

The above shows how it looks to me. Could you share a screenshot of how it looks on your end?

#2 @wonderboymusic
11 years ago

yeah - so I think the issue is non-post-format'd media

#3 @obenland
11 years ago

Right, with an embed as the last element that happens to not be wrapped in a paragraph.

#4 @obenland
11 years ago

Is this a 2013 issue or an inconsistency in how embeds are output?

#5 @wonderboymusic
11 years ago

I was testing the MediaElement upgrade (still hasn't landed) in the past 5 themes, my notes tell me that 2013 was the only problem. I then noticed that the problem persisted without the upgrade.

@amcreative
11 years ago

Screenshot with video from WordPress.tv

#6 in reply to: ↑ description @amcreative
11 years ago

I was not able to replicate this issue. Above is a screenshot using a video from WordPress.tv as a standard post and a video post format. I also embedded a video from Youtube with similar results.

#7 @kraftbj
11 years ago

I could replicate it when using a [video]or [audio] shortcode. YouTube oEmbed isn't impacted since it places the video within <p> tags.

The <div> tags for the embeds are what's lacking.

@mikeyarce
11 years ago

fix bottom margin for video, videopress, and audio embeds

#8 @kraftbj
11 years ago

@mikeyarce - could you update the patch to be relative to root? (e.g. for src/wp-content/themes/twentythirteen/style.css instead of simply style.css)

Makes it easier to test (e.g. grunt patch:29146 would be able to apply the patch). :)

@mikeyarce
11 years ago

updated path

#9 @kraftbj
11 years ago

  • Keywords has-patch added; needs-patch removed

@ccprice
10 years ago

#10 @wonderboymusic
10 years ago

Helped a few of my colleagues navigate this ticket - the CSS rules need to be scoped to standard post format when the audio or video shortcode is not wrapped in a <p>

#11 @obenland
10 years ago

Looking at 29146.3.diff:
We currently don't have a Post Format section for standard post formats in style.css, so we should probably add that and combine the styles. Does the selector have to be that specific? Wouldn't .format-standard .wp-video suffice?

I also wonder why is this only a bug in Twenty Thirteen, how do other default themes handle that scenario?

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


10 years ago

#13 @obenland
10 years ago

  • Milestone changed from 4.0 to Future Release
  • Version changed from trunk to 3.6

Let's look at it in 4.1 again.

@DavidTheMachine
10 years ago

change filepaths so patch will apply

#14 @DavidTheMachine
10 years ago

I'm currently researching obenland's question in comment comment:11, and discovered that patch 29146.3 wasn't applying, so I fixed it.

Last edited 10 years ago by DavidTheMachine (previous) (diff)

#15 @DavidTheMachine
10 years ago

In response to comment:11:

I imported theme-unit-test.xml into the developer install, and checked the default themes as released with 4.0.

The table below shows whether the elements have a bottom margin applied by the style.

twentyten twentyeleven twentytwelve twentythirteen twentyfourteen
Post Format: Audio no no yes yes yes
Post Format: Video (WordPress.TV) yes no yes no -
Post Format: Video (VideoPress) - - no no no
Post Format: Video (YouTube) yes yes yes yes yes

- Theme does not support the underlying shortcode, even with Jetpack installed

Last edited 10 years ago by DavidTheMachine (previous) (diff)

#16 @DrewAPicture
10 years ago

  • Owner set to mikeyarce
  • Status changed from new to assigned

#17 @mikeyarce
10 years ago

@kraftbj and I will be looking at this ticket in the next week and putting together the final patches for all default themes.

#18 @obenland
10 years ago

Is this a themes issue though, or an issue with the different ways that videos are output?

#19 @kraftbj
10 years ago

@obenland - Not totally sure. We'll check that out. Either way, aiming to have something to test in time for Beta 1.

@mikeyarce
10 years ago

Fixes bottom margin in Twenty Ten, Twenty Eleven, and Twenty Thirteen for the Video Player, VideoPress, and Audio Player elements in the Standard Post Format. Tested all default themes up to Twenty Fifteen - all other themes either have top margin in the footer .entry-meta or aren't affected.

#20 @lancewillett
10 years ago

  • Keywords needs-testing added
  • Milestone changed from Future Release to 4.1

If another person could apply latest patch and test -- and report back, we can get this in.

@gregrickaby
10 years ago

Includes all fixes in .5, includes support for .format-audio

#21 @balbert
10 years ago

applied 29146.6.diff and appears to be working in TwentyTen, TwentyEleven and TwentyThriteen for both video and audio players. I tested using the theme unit test data for post lists and video and audio post formats.

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


10 years ago

#23 @lancewillett
10 years ago

  • Keywords commit added; needs-testing removed

#24 @lancewillett
10 years ago

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

In 30371:

Twenty Ten, Eleven, Thirteen: add bottom margin to audio and video players.

Fixes #29146. Props mikeyarce, gregrickaby, DavidTheMachine, jeanyoungkim, ccprice.

Note: See TracTickets for help on using tickets.