Add hero videos (YouTube/Vimeo) to single.html layout #781
Conversation
- Fallback to default positioning for browser that don't support it IE, Chrome < 56, etc. - Issue mmistakes#752
There was a problem hiding this comment.
Thanks for the PR! Some feedback below. If you wouldn't mind fixing some of these things to comply with code style of the theme I can get this merged in.
- Submit pull request against the
developbranch and notmaster. - Fix indenting in (2 space vs. 4)
- Move inline CSS to Sass partial (
_sass/_utlities.scss) - Replace apostrophes with double quotes ie:
<div class="embed-container">instead of<div class='embed-container'>
| {% capture video_id %}{{ page.header.video.id }}{% endcapture %} | ||
| {% capture video_provider %}{{ page.header.video.provider }}{% endcapture %} | ||
|
|
||
| <div class="page__hero{% if page.header.overlay_color or page.header.overlay_video_path %}--overlay{% endif %}" |
There was a problem hiding this comment.
What is the purpose of this code? As a test I added the following YAML Front Matter to add a color overlay as the code seems to suggest is possible, and no video was embedded.
header:
video:
provider: "youtube"
id: "gOBj8HdfA2Y"
overlay_color: "#333"Had to remove overlay_color for it to work.
| > | ||
|
|
||
| {% if video_provider == "vimeo" %} | ||
| <style>.embed-container { position: relative; padding-bottom: 56.25%; height: 0; overflow: hidden; max-width: 100%; } .embed-container iframe, .embed-container object, .embed-container embed { position: absolute; top: 0; left: 0; width: 100%; height: 100%; }</style><div class='embed-container'><iframe src='http://player.vimeo.com/video/{{ video_id }}' frameborder='0' webkitAllowFullScreen mozallowfullscreen allowFullScreen></iframe></div> |
There was a problem hiding this comment.
Fix indent (use 2 spaces), replace single quotes with double, and pull out <style> for .embed-container.
I'd prefer any styling be added to the theme's CSS instead of adding inline. Since it's exactly the same for Youtube and Vimeo this is a safe move.
You can add it to _sass/_utilities.scss at the very end like so:
/*
Responsive Video Embed
========================================================================== */
.embed-container {
position: relative;
padding-bottom: 56.25%;
height: 0;
overflow: hidden;
max-width: 100%;
iframe,
object,
embed {
position: absolute;
top: 0;
left: 0;
width: 100%;
height: 100%;
}
}| > | ||
|
|
||
| {% if video_provider == "vimeo" %} | ||
| <style>.embed-container { position: relative; padding-bottom: 56.25%; height: 0; overflow: hidden; max-width: 100%; } .embed-container iframe, .embed-container object, .embed-container embed { position: absolute; top: 0; left: 0; width: 100%; height: 100%; }</style><div class='embed-container'><iframe src='http://player.vimeo.com/video/{{ video_id }}' frameborder='0' webkitAllowFullScreen mozallowfullscreen allowFullScreen></iframe></div> |
There was a problem hiding this comment.
This is invalid Liquid for "else if". Should be {% elsif video_provider == "youtube" %} instead.
mmistakes
changed the title
|
Cool, thanks for taking the time to educate me :-) Will push updates this weekend - happy Friday! |
justintoo
force-pushed
the
add-hero-video
branch
from
0cb1026 to
c55d436
Compare
No description provided.