WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#21480 closed enhancement (wontfix)

Twenty Twelve: use FitVids to resize videos in small screens

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

Description (last modified by lancewillett)

Currently videos in TwentyTwelve don't look so hot when the view window is scaled down. The problem can be accentuated if you used an embed code that is much wider than the content area. To replicate, try pasting this embed from YouTube into any page or post:

http://www.youtube.com/watch?v=a1Y73sPHKxw

I've never extensively looked into HTML/CSS-only solutions for this, but from my limited looking around it seems to be an unsolved issue for responsive sites. However, there is indeed a jQuery solution called FitVids that handles this very well: https://github.com/davatron5000/FitVids.js. This was mentioned by @chriswallace in #21382. FitVids was overkill for his particular issue, but I still think it warrants being added to TwentyTwelve barring discovery of a simpler option.

Patch is incoming.

Attachments (3)

fitvids.diff (3.7 KB) - added by andyadams 5 years ago.
Howabout a version that includes FitVids?
fitvids2.diff (1.8 KB) - added by rfair404 5 years ago.
Don't target #main on fitvids
21480.patch (2.5 KB) - added by lancewillett 5 years ago.
Non-JS method to wrap and style embeds

Download all attachments as: .zip

Change History (19)

#1 @andyadams
5 years ago

  • Summary changed from FitVids for TwentyTwelve to Twenty Twelve: FitVids for TwentyTwelve

#2 @lancewillett
5 years ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 3.5
  • Summary changed from Twenty Twelve: FitVids for TwentyTwelve to Twenty Twelve: use FitVids to resize videos in small screens

#3 @andyadams
5 years ago

So I goofed on the example - originally I had an iframe with inline widths and heights, but that issue is not fixed in FitVids (I have a patch that needs to be approved).

In the meantime, try using this URL in your page or post to see the issue:

http://www.youtube.com/watch?v=Cu20ele1ojQ

#4 @andyadams
5 years ago

  • Keywords has-patch added

@andyadams
5 years ago

Howabout a version that includes FitVids?

#6 in reply to: ↑ 5 @andyadams
5 years ago

Replying to obenland:

Are you sure this wasn't fixed with [21408]?

Have a look at: http://responsinator.com/?url=themebuster.wordpress.net%2Fpost-format-test-video%2F

Here's what I see using Chrome:

http://img.thethemefoundry.com/video-aspect-ratio-1.png

The black bars above/below are what I think FitVids fixes.

#7 @ryanimel
5 years ago

  • Cc ryan@… added

After reading through the FitVids script, I think I would prefer a method like what this plugin is going: http://wordpress.org/extend/plugins/responsive-video-embeds/

Similar methods -- looking for embeds, wrapping them in a container and applying CSS to each to make the responsiveness work. But cribbing the plugin's way wouldn't require Javascript to work. Worth consideration/testing more, I think.

#8 @bradthomas127
5 years ago

The only problem i see with FitVids.js is that if someone wants to use the embed code for a smaller video and add class="alignright" to it so they can wrap it in text, FitVids.js will override the size and make it 100% of the container. I guess you could always wrap the embed code with a div though.

@rfair404
5 years ago

Don't target #main on fitvids

#9 @rfair404
5 years ago

  • Cc rfair404 added

I implemented Fitvids in a plugin before and wound up taking an approach similar to the diff. Instead of running fitvids on #main add a post class and target that instead.

This would do a couple of things...
A) a child theme could easily break this feature by using a div id other than main
B) it is much easier to short circuit (if fo some reason the fitvids feature is unwanted) by remove_filter

#10 @lancewillett
5 years ago

After looking at this more it seems like loading more JS isn't worth the trade-off for the quick visual height issue. While that looks bad, once you play the video the height and aspect ratio are correct.

The plugin Ryan mentioned looks promising, though, since it's a PHP + CSS solution.

@lancewillett
5 years ago

Non-JS method to wrap and style embeds

#11 follow-ups: @obenland
5 years ago

@lancewillett: Only found a syntax error with a comma missing after 'scribd.com'. Do we really need to remove width and height? I found it working in my tests without the entire replace part. We could then just check the whitelist and wrap the html in the if statement. Leaves us with only one return. I could be wrong on that, though.


On the issue in general:
There will never be a holistic solution either way.

  • Apart from the extra request, Fitvids only supports four core embed providers out of the box.
  • With PHP/CSS: Providers added through the 'oembed_providers' filter won't get the treatment. And while adding a filter to the whitelist in twentytwelve_modify_embed_output() might help, we'll still have to keep the provider whitelist up-to-date with the default providers in core, once it's in.

I don't see why this has to be "fixed" in the first place. And I don't like the overhead of either solution for such a (in my eyes) small gain.

#12 @betzster
5 years ago

  • Cc j@… added

#13 in reply to: ↑ 11 @lancewillett
5 years ago

Replying to obenland:

@lancewillett: Only found a syntax error with a comma missing after 'scribd.com'. Do we really need to remove width and height? I found it working in my tests without the entire replace part. We could then just check the whitelist and wrap the html in the if statement. Leaves us with only one return. I could be wrong on that, though.

Hmm, it's worth exploring removing the width/height checks. I'll test some more. Good catch on the missing comma, saw that after I put up my patch.

I don't see why this has to be "fixed" in the first place. And I don't like the overhead of either solution for such a (in my eyes) small gain.

Heh, I think it's a better experience overall to see the video in the correct size before it plays.

Your point on tracking new embed sources in core is valid -- ideally this HTML wrapper would come from core, not the theme. That way any theme can benefit from the extra hook.

I feel the same way about fitvids, it should be on the platform (core, video providers, or in browsers natively).

#14 in reply to: ↑ 11 @lancewillett
5 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

Replying to obenland:

  • With PHP/CSS: Providers added through the 'oembed_providers' filter won't get the treatment. And while adding a filter to the whitelist in twentytwelve_modify_embed_output() might help, we'll still have to keep the provider whitelist up-to-date with the default providers in core, once it's in.

@obenland Do you want to research this a bit in core? See if there's an existing ticket for oembed extra wrapper element with a class value, if not -- maybe add one for 3.5.

I think it doesn't make sense for a solution like this to be in Twenty Twelve or any specific theme -- except for maybe the CSS for positioning, but that could come from a plugin or core embed as well.

Closing ticket as wontfix.

#15 @helenyhou
5 years ago

  • Milestone 3.5 deleted

#16 @cais
5 years ago

  • Cc edward.caissie@… added
Note: See TracTickets for help on using tickets.