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

[WIP] Feature/video elements #1348

Merged
merged 3 commits into from
Jun 14, 2018
Merged

[WIP] Feature/video elements #1348

merged 3 commits into from
Jun 14, 2018

Conversation

frontendschlampe
Copy link
Contributor

add some more options for audio and video elements:

Audio/Video:

  • add option to hide controls
  • add option to play media in loop
  • add option to mute media
  • add option playsinline to prevent automatic fullscreen mode on iPhone devices when playback begins
  • add options for preloading
  • add options for start at and end at
  • add possibility to include an audio/video caption
  • add option to make audio/video responsive (using full width of its container)

YouTube

  • add option to hide controls
  • add option using extended privacy (via youtube-nocookie.com)
  • add possibility to include a video caption
  • add option to make video responsive (using full width of its container)

Vimeo

  • add option to play media in loop
  • add option to hide the user’s portrait on the video
  • add option to hide the title on the video
  • add option to hide the byline on the video
  • add option for start at (end at is not supported by Vimeo)
  • add possibility to include a video caption
  • add option to change color
  • add option to make video responsive (using full width of its container)

Many thanks to my customers for their financial support of this pr.

@leofeyer
Copy link
Member

leofeyer commented Feb 6, 2018

Again, please rebase. 😉

@frontendschlampe
Copy link
Contributor Author

ok ... which branch?

@xchs
Copy link
Contributor

xchs commented Feb 6, 2018

Pull Requests for new features belong to the master branch.

@frontendschlampe
Copy link
Contributor Author

We just extend an existing feature ;-)

@ausi
Copy link
Member

ausi commented Feb 6, 2018

This pull request has the feature label as you can see in the right column 🤓

@frontendschlampe
Copy link
Contributor Author

frontendschlampe commented Feb 6, 2018

Did I do it correctly with rebase?

@leofeyer
Copy link
Member

You are adding 15 (!) new fields to tl_content. Can this not be optimized by sharing certain fields?

@leofeyer leofeyer changed the title Feature/video elements [WIP] Feature/video elements Feb 21, 2018
@frontendschlampe
Copy link
Contributor Author

I will talk to @discordier and we will try ...

@frontendschlampe
Copy link
Contributor Author

I talked to @discordier and we can share the fields, but we adopted the fields from c7b4f38 - there's a prefix for "youtube" options. When we want to merge the options, the prefix should be removed, which will be a problem for future updates. We planed a default prefix like "video", but we were to late. So we have to make a update script for renaming the existing fields?!

@Aybee
Copy link
Contributor

Aybee commented Feb 21, 2018

Should be one widget with multiple checkboxes I think. If this is possible.

@frontendschlampe
Copy link
Contributor Author

Should be one widget with multiple checkboxes I think. If this is possible.

I think, this is not possible, because we already have 3 different CE's ... maybe in Contao 5 we can make only one CE

@leofeyer
Copy link
Member

leofeyer commented Mar 6, 2018

Here's how I think that the fields should be:

  • playerOptions: HTML5 player options
  • youtubeOptions: YouTube options
  • vimeoOptions: Vimeo options
  • playerPreload (shared between all player elements)
  • playerStart (shared between all player elements)
  • playerStop (shared between all player elements)
  • playerCaption (shared between all player elements)
  • playerAspect (shared between all player elements)
  • playerColor (shared between all player elements)

So the following fields need to be removed:

  • playerResponsive: should be an option in playerOptions
  • youtubeCaption: use playerCaption instead
  • youtubeResponsive: should be an option in youtubeOptions
  • youtubeAspect: use playerAspect instead
  • youtubeStart: use playerStart instead
  • youtubeStop: use playerStop instead
  • vimeoStart: use playerStart instead
  • vimeoStop: use playerStop instead
  • vimeoCaption: use playerCaption instead
  • vimeoColor: use playerColor instead
  • vimeoResponsive: should be an option in vimeoOptions
  • vimeoAspect: use playerAspect instead

If an element does not support one of the shared fields, we simply hide it.

@leofeyer
Copy link
Member

@frontendschlampe What's the status on this?

@frontendschlampe
Copy link
Contributor Author

yes ... I don't forget it! I hope, that I will work on it with @discordier this week ... not later than the beginning of next week.

@leofeyer leofeyer added this to the 4.6.0 milestone May 17, 2018
@@ -103,7 +103,7 @@
// Palettes
'palettes' => array
(
'__selector__' => array('type', 'addImage', 'sortable', 'useImage', 'overwriteMeta', 'protected'),
'__selector__' => array('type', 'addImage', 'sortable', 'useImage', 'overwriteMeta', 'protected', 'youtubeResponsive', 'vimeoResponsive'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be 'playerResponsive', doesn't it?

@leofeyer
Copy link
Member

I wonder if it makes sense to have the video-related CSS code in a separate file (see contao-components/contao#10)? The image-related CSS code is in the layout.css style sheet, so why don't we just add the video-related code there, too?

@contao/developers /cc

@Toflar
Copy link
Member

Toflar commented Jun 12, 2018

Fine for me. Is there already a PR for migrations because like this it's a BC break atm.

@Toflar
Copy link
Member

Toflar commented Jun 12, 2018

And I wonder if we should drop autoplay completely. It's always been a bad idea and browser vendors are finally starting to block them automatically so it has no purpose anymore anyway.

@aschempp
Copy link
Member

And I wonder if we should drop autoplay completely.

Really? I see a lot of background videos playing automatically. And that sounds like a useful case?

@Toflar
Copy link
Member

Toflar commented Jun 12, 2018

@frontendschlampe
Copy link
Contributor Author

Autoplay with sound is forbidden since a long time. If you want to have autoplay, you also have to play your video muted. But I wouldn't drop autoplay.

@frontendschlampe
Copy link
Contributor Author

I wonder if it makes sense to have the video-related CSS code in a separate file (see contao-components/contao#10)? The image-related CSS code is in the layout.css style sheet, so why don't we just add the video-related code there, too?

I put in a separate one, because you need it only if you want to use responsive video. If you don't want to use responsive video, you don't have to use any separate css.

@leofeyer
Copy link
Member

But following this logic, we would also have to remove the responsive images code from the responsive.css file, wouldn't we?

@frontendschlampe
Copy link
Contributor Author

But following this logic, we would also have to remove the responsive images code from the responsive.css file, wouldn't we?

So it's not in the layout.css but in the responsive.css - I think the best would be, we add it to responsive.css, isn't it? So all responsive styles are at one place!

@Aybee
Copy link
Contributor

Aybee commented Jun 12, 2018

As I e.g. do not use Contao FE CSS for me it's nice to have this CSS per Checkbox. But I also can put this CSS into my own CSS file like I do with responsive images

img {
  max-width: 100%;
  height: auto;
}

/* default 16:9 aspect ratio */
.video-wrapper {
  position: relative;
  padding-bottom: 56.25%;
  height: 0;
}
/* 21:9 aspect ratio */
.video-wrapper.ar219 {
  padding-bottom: 42.8571%;
}
/* 16:10 aspect ratio */
.video-wrapper.ar1610 {
  padding-bottom: 62.5%;
}
/* 4:3 aspect ratio */
.video-wrapper.ar43 {
  padding-bottom: 75%;
}
/* 1:1 aspect ratio */
.video-wrapper.ar11 {
  padding-bottom: 100%;
}
/* span iframe */
.video-wrapper > * {
  position: absolute;
  top: 0;
  left: 0;
  width: 100%;
  height: 100%;
  border: none;
}

leofeyer added a commit to contao-components/contao that referenced this pull request Jun 13, 2018
leofeyer added a commit to contao-components/contao that referenced this pull request Jun 13, 2018
@leofeyer leofeyer merged commit 0d0aba2 into contao:master Jun 14, 2018
@leofeyer
Copy link
Member

Thank you @frontendschlampe.

leofeyer added a commit to contao/installation-bundle that referenced this pull request Jun 14, 2018
@frontendschlampe frontendschlampe deleted the feature/video-elements branch June 14, 2018 07:20
@qzminski
Copy link
Member

This commit brings back the problem with frameborder attribute (see #1233). Can this be updated before releasing 4.6.0?

@leofeyer
Copy link
Member

Removed in eaaca08.

@frontendschlampe
Copy link
Contributor Author

This commit brings back the problem with frameborder attribute (see #1233). Can this be updated before releasing 4.6.0?

That's interesting ... then YouTube and Vimeo providing invalid code. ;-)

@leofeyer
Copy link
Member

IDTS. I may have copied it at some point.

@frontendschlampe
Copy link
Contributor Author

<iframe width="560" height="315" src="https://www.youtube.com/embed/Rw8iMnkqvwk" frameborder="0" allow="autoplay; encrypted-media" allowfullscreen></iframe>

Is original embed code from YouTube. ;-)

@leofeyer
Copy link
Member

😳

@qzminski
Copy link
Member

I guess they provide it for regular everyday normal users that only copy-paste the code, so that they are not confused with the frame border which is default styling for some browsers. The W3C clearly says it's an error however:

2018-06-25 at 15 17

… and even points to the Wikipedia page that provides a list of equivalents.

christian-kolb pushed a commit to christian-kolb/contao that referenced this pull request Oct 11, 2018
leofeyer added a commit that referenced this pull request Feb 18, 2020
Description
-----------

See #1307 

### Before

<img width="568" alt="" src="https://user-images.githubusercontent.com/1192057/74720064-718c8400-5235-11ea-81f6-3d164fbfc22e.png">

### After

<img width="592" alt="" src="https://user-images.githubusercontent.com/1192057/74720079-78b39200-5235-11ea-93b1-9f310e1e4ca9.png">

Commits
-------

c9c7671f Correctly align the wizard icon (see #1307)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants