Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

Responsive embeds and heading line height #615

Merged
merged 4 commits into from
Mar 17, 2016

Conversation

jaicab
Copy link
Contributor

@jaicab jaicab commented Mar 15, 2016

The YouTube embeds on the website weren't responsive, causing a increase in layout width on mobile, breaking the experience. Using the common pattern of 16:9 aspect ratio for responsive embeds via CSS I applied it to all the iframes in the HTML.

I also fixed a line height problem in the headings as it was being relied on for margin. In cases where the headings go to two lines, the spacing was huge. So changed it to 1 and applied margin bottom.

@cco3
Copy link
Contributor

cco3 commented Mar 16, 2016

LGTM, but I'm going to let Matthew take a look at this one.

@scottjenson
Copy link
Contributor

This is VERY odd CSS. Doing a little research, it is considered standard practice and there are multiple articles that show this exact code. However, I did notice in these articles that much of this code is to get around IE5 and IE6 . First, there aren't many mobile browsers that use IE5 or 6 and second, most folks have moved well beyond supporting such legacy browsers. These same articles point out that the following mark up is what any sane browser should use:

`video {
width: 100% !important;
height: auto !important;
}

Instead of supporting CopyPastaCSS, This HTML5 solution seems significantly simpler and allows for a single change to the CSS file. Any objections to using that approach?

@jaicab
Copy link
Contributor Author

jaicab commented Mar 17, 2016

Correct me if I'm wrong, but I think that would only work if the YouTube embeds were video elements, but they are iframes.
I'm confident there's no way to embed YouTube videos using video without violating their terms of service. One solution is of course, to self-host the videos somehow and have access to the MP4 and WebM files for them.

Applying that CSS to an iframe would result in an embed with fluid width but fixed height of 150px, even if we removed the height and width attributes from the iframes. The problem is that while video is aware of the aspect ratio of a source, an iframe isn't aware of its contents.

The reason the iframe would have a fixed height is because of the 2:1 ratio used on iframes by default (as mentioned in the CSS2 spec), with a default width of 300px the resultant min/default height would be 150px. It would scale down on viewport widths smaller than 300px, but never scale up, and the ratio would still be 2:1 and not 16:9 like in the videos.

I hope this reflects it's not about CopyPastaCSS, I think I know all the possible ways of implementing responsive videos but went with the one that fit the scenario and had the least impact in the codebase. I did use a classname commonly related with this pattern so other people contributing could recognise it more easily.

I would say that we don't really need the object and embed parts in the CSS though, just the iframe.

@scottjenson
Copy link
Contributor

Thank you @jaicab that makes sense and I very much appreciate your experience on this. CSS just has a way of making you think it's a hack when as you point out, it may be the only way to do it!

Just to confirm, are you saying the current pull request is the best way, or is there a small modification you could make. I'm happy to accept either version.

@jaicab
Copy link
Contributor Author

jaicab commented Mar 17, 2016

No problem @scottjenson, happy to help! I love the project.

I've done that small modification, which is just to remove the embed and object from the selector, as it's only iframe that we need, as we have no embeds or objects on the page.

It's ready to merge if you ask me!

scottjenson added a commit that referenced this pull request Mar 17, 2016
Responsive embeds and heading line height
@scottjenson scottjenson merged commit 19d0cc0 into google:gh-pages Mar 17, 2016
@jaicab jaicab deleted the responsive-embed branch March 17, 2016 16:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants