Skip to content

Add filter to allow more video types in AMP story background selection#3171

Merged
swissspidy merged 10 commits intoampproject:developfrom
thrijith:add/filter-forvideo-extension
Sep 9, 2019
Merged

Add filter to allow more video types in AMP story background selection#3171
swissspidy merged 10 commits intoampproject:developfrom
thrijith:add/filter-forvideo-extension

Conversation

@thrijith
Copy link
Copy Markdown
Contributor

@thrijith thrijith commented Sep 3, 2019

Fixes #3168

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no Has not signed the Google CLA label Sep 3, 2019
@thrijith
Copy link
Copy Markdown
Contributor Author

thrijith commented Sep 3, 2019

@googlebot I signed it!

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Sep 3, 2019
@swissspidy
Copy link
Copy Markdown
Collaborator

@thrijith I took the liberty to make some changes directly so you can better see the direction I think this should take. Let me know what you think.

I also saw that we still have many areas where video/mp4 is hardcoded. For example in our custom video block edit component.

@thrijith
Copy link
Copy Markdown
Contributor Author

thrijith commented Sep 5, 2019

I took the liberty to make some changes directly so you can better see the direction I think this should take. Let me know what you think.

Thanks @swissspidy 🙂, I was not sure, how to go about adding changes needed for store.

I also saw that we still have many areas where video/mp4 is hardcoded. For example in our custom video block edit component.

Are you referring the accept part?

onSelect={ onSelectVideo }
onSelectURL={ this.onSelectURL }
accept="video/mp4"
allowedTypes={ ALLOWED_VIDEO_TYPES }

It doesn't allow selection for other extensions when using Upload button in the Video block.

@thrijith
Copy link
Copy Markdown
Contributor Author

thrijith commented Sep 5, 2019

index 412fabac..d45a5580 100644
--- a/assets/src/stories-editor/components/custom-video-block-edit.js
+++ b/assets/src/stories-editor/components/custom-video-block-edit.js
@@ -235,7 +235,7 @@ class CustomVideoBlockEdit extends Component {
 					className={ className }
 					onSelect={ onSelectVideo }
 					onSelectURL={ this.onSelectURL }
-					accept="video/mp4"
+					accept={ allowedVideoMimeTypes.join( ',' ) }
 					allowedTypes={ allowedVideoMimeTypes }
 					value={ this.props.attributes }
 					notices={ noticeUI }
diff --git a/assets/src/stories-editor/components/with-enforced-video-upload-type.js b/assets/src/stories-editor/components/with-enforced-video-upload-type.js
index 39c49239..7520580c 100644
--- a/assets/src/stories-editor/components/with-enforced-video-upload-type.js
+++ b/assets/src/stories-editor/components/with-enforced-video-upload-type.js
@@ -14,11 +14,11 @@ import PropTypes from 'prop-types';
  */
 export default ( InitialMediaPlaceholder ) => {
 	const withEnforcedVideoUploadType = ( props ) => {
-		const { accept, className } = props;
+		const { accept, className, allowedTypes } = props;
 		let newProps = { ...props };
 
 		if ( 'wp-block-video' === className && 'video/*' === accept ) {
-			newProps = { ...props, accept: 'video/mp4' };
+			newProps = { ...props, accept: allowedTypes.join( ',' ) };
 		}
 
 		return <InitialMediaPlaceholder { ...newProps } />;
@@ -27,6 +27,7 @@ export default ( InitialMediaPlaceholder ) => {
 	withEnforcedVideoUploadType.propTypes = {
 		accept: PropTypes.string,
 		className: PropTypes.string,
+		allowedTypes: PropTypes.arrayOf( PropTypes.string ).isRequired,
 	};
 
 	return withEnforcedVideoUploadType;

Since allowed video type is available in props, we could do something like this to remove usage of hardcoded mime type from custom video block element.

@swissspidy
Copy link
Copy Markdown
Collaborator

with-enforced-video-upload-type.js is probably going away soon as we don't really need that anymore now that we have custom-video-block-edit.js - see #3189.

Otherwise your suggested change looks good 👍

@thrijith
Copy link
Copy Markdown
Contributor Author

thrijith commented Sep 6, 2019

I have updated the accept attribute value for custom video edit block, I think the PR can be merged, as it allows selection of other mime type in Background Media as well as video block. Please let me know if it needs any other change.

@swissspidy swissspidy added this to the v1.3 milestone Sep 6, 2019
@swissspidy swissspidy self-requested a review September 6, 2019 10:35
Copy link
Copy Markdown
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Approved from my side, but requesting 1 more review as I did add lots of changes myself here.

Copy link
Copy Markdown
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

👍

@swissspidy swissspidy merged commit 0a6f6dd into ampproject:develop Sep 9, 2019
@thrijith thrijith deleted the add/filter-forvideo-extension branch September 9, 2019 16:32
@swissspidy swissspidy mentioned this pull request Sep 9, 2019
8 tasks
westonruter added a commit that referenced this pull request Sep 10, 2019
…ode-support

* 'develop' of github.com:ampproject/amp-wp: (28 commits)
  Exclude development files from production build ZIPs
  Add filter to allow more video types in AMP story background se… (#3171)
  Update dependency @babel/cli to v7.6.0 (#3203)
  Update dependency @babel/core to v7.6.0 (#3204)
  Remove deprecated AMP_WP_Utils class
  Refresh Composer lock file
  Run build in a temporary folder
  Add shell script to clean up the current folder and then trigger a build
  Adapt Gruntfile to fetch optimized Composer dependencies within the build folder
  Retrieve Sabberworm patch directly from the GitHub PR
  Adding regression test for vertical text alignment issues
  Fixing minor errors in test descriptions
  Only restrict height inside non-fit text blocks
  Rewritten block size tests
  Limit height declaration to only direct descendant
  Remove now obsolete withEnforcedVideoUploadType HOC
  Update eslint-plugin-jest and apply fixes (#3192)
  Update dependency postcss to v7.0.18 (#3200)
  Update dependency terser-webpack-plugin to v2 (#3195)
  Update dependency webpack-cli to v3.3.8 (#3194)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Signed the Google CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[AMP Stories] Add filter for allowing more video extensions

5 participants