Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Video support improvements #81

Merged
merged 8 commits into from
Sep 18, 2016
Merged

Video support improvements #81

merged 8 commits into from
Sep 18, 2016

Conversation

obilodeau
Copy link
Member

@obilodeau obilodeau commented Aug 28, 2016

Made several improvements to the support and documentation around videos. Fixes #71.

  • background videos on individual slides is supported (w/ shorthand options)
  • videos by default will use a saner size ([.stretch])
  • everything is documented

The PR is larger than it should because I branched it from data-background-rework and rebased it from master before pushing. Once #52 is merged, I'll rebase again on master and reviewing should be a lot more straightforward.

Task list:

  • merge PR data-background attributes rework #52
  • Try to find another solution than .stretch class to make video large (100% or cover/contain sizes?)
  • issue: inline video options="autoplay" makes the video start at prefetch time which is too soon
  • re-wrap documentation to one sentence per line

@mojavelinux
Copy link
Member

Great work!

videos by default will use a saner size ([.stretch])

I'd prefer to stick to the terminology from CSS where possible. CSS offers two sizing keywords, "contain" and "cover". They both give you ways to fit the object to the canvas. I think we should use those names instead (in fact, it might even be better to use CSS rules instead of width and height attributes).

@obilodeau
Copy link
Member Author

I haven't made up the name. The .stretch class is provided by reveal.js and is applied via javascript to the HTML tag with it.

That said, you are right, we should try to have a CSS solution instead.

@obilodeau
Copy link
Member Author

Added task:

  • issue: inline video options="autoplay" makes the video start at prefetch time which is too soon

@obilodeau
Copy link
Member Author

Fixed video options=autoplay.

  • Doesn't work on vimeo videos. This is an upstream issue.
  • I tried adding data-autoplay to a background video section but it didn't show up in the HTML... Needs further investigation.

@mojavelinux
Copy link
Member

I wasn't aware Reveal.js provided the stretch class. In that case, we do want to be able to provide access to all underlying features, so 👍 to supporting both ways of expressing it.

@obilodeau
Copy link
Member Author

@obilodeau
Copy link
Member Author

I have tried several CSS instructions and I wasn't able to achieve the same result as what revealjs' stretch class does on youtube iframes. I tried several combinations of min-height, height and positioning things and nothing worked. However I have to admit I'm not that skilled with CSS...

@mojavelinux any idea what should be done to avoid revealjs' stretch class here? For context, that solution relies on javascript to alter the DOM to specify the height and width (in pixels) to the div containing the iframe.

Other than that, the stretch class is applied transparently for the user. We don't introduce a keyword. So if we don't find a CSS solution, I think this should go in as-is.

@obilodeau obilodeau changed the title [WIP] Video support improvements Video support improvements Sep 5, 2016
@obilodeau obilodeau merged commit ac7fbb1 into master Sep 18, 2016
@obilodeau obilodeau deleted the video-improvements branch October 2, 2016 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants