Background Media support: relevant Video updates + basic implementation #194
Conversation
Signed-off-by: Miina Sikk <miina.sikk@gmail.com>
Signed-off-by: Miina Sikk <miina.sikk@gmail.com>
|
@dvoytenko @spacedmonkey @swissspidy This is partial for #25 -- since the video edit mode wasn't implemented previously but panning is necessary for the background media, then this PR currently implements basic background setting (button + when adding media to an empty page) and the required parts for video element. Thinking that perhaps we could get these merged and then address the remaining issues (listed out in the description) separately to avoid the PR getting too huge. Thoughts? |
assets/src/edit-story/components/canvas/singleSelectionMovable.js
Outdated
Show resolved
Hide resolved
| @@ -43,9 +43,16 @@ | |||
| <noscript><style amp-boilerplate="">body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> | |||
| <?php /* todo: include custom CSS via PHP */ ?> | |||
| <style amp-custom> | |||
| .full-bleed > :first-child { | |||
| .is-fill > :first-child { | |||
There was a problem hiding this comment.
Not for this PR, but I'm curious, why shouldn't we generate the <style amp-custom> (or even the rest of this document) entirely in our JS code?
There was a problem hiding this comment.
Currently, this template is using WordPress filters and functions to use and display relevant data from the site. Also, previously the amp-custom was put together automatically after tree-shaking, combining all the relevant styles. Not knowing if this will be the case in the future as well, amp-custom was just added as a quick replacement when separating the Stories plugin from AMP. Basically -- this part has just never been seriously discussed in terms of how to store/generate, it might be good to do that before the first release.
For portability, it would be more convenient for example, if the amp-story tag would also be saved together with the content, currently, only the pages inside amp-story are saved.
| @@ -35,10 +35,24 @@ function VideoPreview( { id, mimeType, src, width, height, x, y, rotationAngle, | |||
| id: 'el-' + id, | |||
| }; | |||
|
|
|||
| const style = getCommonAttributes( { width, height, x, y, rotationAngle } ); | |||
| const style = isBackground ? | |||
There was a problem hiding this comment.
This logic is repeated.
assets/src/edit-story/app/story/useStoryReducer/reducers/setBackgroundElement.js
Show resolved
Hide resolved
| } : | ||
| getCommonAttributes( { width, height, x, y, rotationAngle } ); | ||
|
|
||
| if ( isBackground ) { |
There was a problem hiding this comment.
Shouldn't we respect the existing autoPlay param passed to this component? Maybe change that instead.
There was a problem hiding this comment.
Good question, not sure. I suppose that's a UX question. It seems like if a video is set as background then it should always autoplay, however, when the background is unset again for that element then I'd assume that the previous autoplay setting would remain as it was before.
Let's confirm with UX, this could also be addressed in #45 once the UX for the Video element is confirmed.
There was a problem hiding this comment.
For some reason I was always under the impression that all videos always autoplay in stories. I'm looking at the doc here, which states:
Note that amp-video extension has different validation rules when used within an AMP Story—both the autoplay and poster attributes are required.
And consequently, I don't think controls are allowed or make much sense.
There was a problem hiding this comment.
Hmm, I think we've had that conversation before with the previous Stories editor as well already.
Yep, here's a relevant PR:
ampproject/amp-wp#2255
So yes, it looks like the controls don't really make sense anyway. Will leave this for resolving/looking into in #45 and related issues and merge as-is for the background media.
Partial for #25:
Todo: