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

[Mobile]Add video block (first iteration) #14912

Merged
merged 42 commits into from May 12, 2019

Conversation

@pinarol
Copy link
Contributor

commented Apr 10, 2019

Description

To Fix: wordpress-mobile/gutenberg-mobile#855

This PR introduces the new video block for Mobile. First of all I am sorry for this huge PR. I will try to add notes and make it easier to understand as much as possible. Please note that most of the green changes aren't actually new code, they are mostly extracted from image block, I added some inline comments to point this out.

How has this been tested?

Tested via: wordpress-mobile/gutenberg-mobile#854

Types of changes

  • Extracted MediaUpload component from Image block (similar to web )
  • Extracted MediaUploadProgress component from Image block, it's now been used by image and video blocks
  • Updated MediaPlaceholder component to be suitable for different media types ( similar to web )
  • Added video block and reused MediaUpload, MediaUploadProgress, MediaPlaceholder components
  • Added react-native-video 3rd party library to have the preview of videos. Also this makes it similar to web side which uses html <video> tag, so hopefully any future abstractions will be easier to make the video block cross platform.

I added some additional inline comments to describe what's done.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@pinarol pinarol requested a review from chrisvanpatten as a code owner Apr 10, 2019

@pinarol pinarol self-assigned this Apr 10, 2019

@pinarol pinarol removed the request for review from chrisvanpatten Apr 10, 2019

@@ -23,6 +23,7 @@ export function initializeEditor() {
if ( typeof __DEV__ === 'undefined' || ! __DEV__ ) {
unregisterBlockType( 'core/code' );
unregisterBlockType( 'core/more' );
unregisterBlockType( 'core/video' );

This comment has been minimized.

Copy link
@pinarol

pinarol Apr 10, 2019

Author Contributor

Added this so we won't see this on production yet

border-bottom-left-radius: $play-icon-size/8;
border-bottom-right-radius: $play-icon-size/8;
border-top-right-radius: $play-icon-size/8;
border-top-left-radius: $play-icon-size/8;

This comment has been minimized.

Copy link
@pinarol

pinarol Apr 10, 2019

Author Contributor

I don't know why but I am getting red screen if I just use border-radius

@@ -0,0 +1,151 @@
/**

This comment has been minimized.

Copy link
@pinarol

pinarol Apr 10, 2019

Author Contributor

This is a new component to be used by both video and image blocks(and probably by other media upload blocks in the future). It is actually not new code, just extracted from image block.

It listens to media upload events coming from main apps and updates progress Spinner. It also provides a way to render a child component with the current upload state(see props.renderContent) If provided a coverUrl then it will also calculate width, height of the image and pass to renderContent.

This comment has been minimized.

Copy link
@etoledom

etoledom May 10, 2019

Contributor

Maybe a better place for this component would be media-upload/? Since it's meant to be shared between media related blocks.
It also looks like a good candidate for a HOC (not 100% sure). This might also make easier its implementation in blocks.

This comment has been minimized.

Copy link
@pinarol

pinarol May 10, 2019

Author Contributor

Thanks @etoledom 👍 I added a tech debt here wordpress-mobile/gutenberg-mobile#687 (comment)

@@ -0,0 +1,98 @@
/**

This comment has been minimized.

Copy link
@pinarol

pinarol Apr 10, 2019

Author Contributor

This is a new component to be used by both video and image blocks also by MediaUpload component. It is actually not new code, just extracted from image block.

This component is responsible for displaying media options picker to user. It is designed similar to what web has, it passes a function 'open' via its render method and the rendering component decides when to call that function.

Different thing is web doesn't need to pass getMediaOptions but we do, the rendering component also needs to embed getMediaOptions() call to an appropriate place. I tried wrapping this component with a View and calling getMediaOptions() inside, but that messed up the layout of edit button. 🤷‍♀️

@@ -8,25 +8,67 @@ import { View, Text, TouchableWithoutFeedback } from 'react-native';
*/
import { __ } from '@wordpress/i18n';
import { Dashicon } from '@wordpress/components';
import { MediaUpload, MEDIA_TYPE_IMAGE, MEDIA_TYPE_VIDEO } from '@wordpress/block-editor';

/**
* Internal dependencies
*/
import styles from './styles.scss';

function MediaPlaceholder( props ) {

This comment has been minimized.

Copy link
@pinarol

pinarol Apr 10, 2019

Author Contributor

We are making MediaPlaceholder usable by both image and video blocks by passing media type.

</Text>
</View>
</TouchableWithoutFeedback>
<MediaUpload

This comment has been minimized.

Copy link
@pinarol

pinarol Apr 10, 2019

Author Contributor

We are also using our new MediaUpload component here.

return (
<View onLayout={ this.onVideoContanerLayout } style={ { flex: 1 } }>
{ showVideo &&
<Video

This comment has been minimized.

Copy link
@pinarol

pinarol Apr 10, 2019

Author Contributor

By using this video component we are coming close to what web side has.

} }
// Using built-in player controls is messing up the layout on iOS.
// So we are setting controls=false and adding a play button that
// will trigger presentFullscreenPlayer()

This comment has been minimized.

Copy link
@koke

koke Apr 11, 2019

Contributor

Sounds good as a workaround for this PR, but let's make sure to track this in another issue for 2.0.

This comment has been minimized.

Copy link
@pinarol

pinarol Apr 12, 2019

Author Contributor

I've added a new item to the TODO list here: wordpress-mobile/gutenberg-mobile#687
Unfortunately this is in library level, they are setting the frame (as a workaround for some issue) when controls=true
https://github.com/react-native-community/react-native-video/blob/c30f246f5694452f4f081ff30ac4523dfd89f41d/ios/Video/RCTVideo.m#L593
And by doing that they are messing up the y position of our page. I couldn't come up with a nice solution myself, I was planning to open an issue to their repo after this is merged. I think they might agree that current way is not very healthy. I can also try to tackle this more and send a PR to the original repo if they ignore the issue in the long run 🤔

@pinarol pinarol requested a review from etoledom Apr 17, 2019

marecar3 and others added some commits Apr 17, 2019

Merge branch 'master' into rnmobile/video-block-first-iteration
# Conflicts:
#	packages/block-editor/src/components/media-placeholder/index.native.js
#	packages/block-library/src/image/edit.native.js

@marecar3 marecar3 requested review from mzorz and daniloercoli Apr 24, 2019

marecar3 and others added some commits Apr 25, 2019

Merge branch 'master' into rnmobile/video-block-first-iteration
# Conflicts:
#	packages/block-editor/src/components/media-placeholder/index.native.js
componentDidMount() {
const { attributes, setAttributes } = this.props;
if ( attributes.id && ! isURL( attributes.src ) ) {
if ( attributes.url.indexOf( 'file:' ) === 0 ) {

This comment has been minimized.

Copy link
@marecar3

marecar3 May 6, 2019

Contributor

Hey @pinarol @SergioEstevao do we need this block of code as it was put for copy/paste images on iOS but not sure for videos?

This comment has been minimized.

Copy link
@pinarol

pinarol May 7, 2019

Author Contributor

It looks like we don't have support for video copy paste on Aztec either. So yes, we don't need this code, let me remove that.

marecar3 and others added some commits May 6, 2019

Remove unnecessary code for video copy/paste handling
We are not supporting video copy paste, that code was copy/paste from image block
Removing poster support
poster support isn't present on wp.com yet and react-native-video has an ugly bug
about posters so we'll need to solve that first, after that we'll re-add this.
@etoledom
Copy link
Contributor

left a comment

Thank you for all this great hard work @pinarol !

It looks great and has been more than tested via wordpress-mobile/gutenberg-mobile#854 and wordpress-mobile/WordPress-iOS#11454 (for iOS).

I left a small code commen as a possible future optimization.

@@ -0,0 +1,151 @@
/**

This comment has been minimized.

Copy link
@etoledom

etoledom May 10, 2019

Contributor

Maybe a better place for this component would be media-upload/? Since it's meant to be shared between media related blocks.
It also looks like a good candidate for a HOC (not 100% sure). This might also make easier its implementation in blocks.

@pinarol pinarol referenced this pull request May 10, 2019

Open

Video block #687

@marecar3

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

Hey @pinarol, as we have refactored an image and media progress upload in this PR I have manually included this fix: #15532 into our PR.

@marecar3 marecar3 merged commit 295a381 into master May 12, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.