Media: Refactor MediaUpload, MediaPlaceholder and mediaUpload to support arrays with multiple supported types. by jorgefilipecosta · Pull Request #9707 · WordPress/gutenberg

jorgefilipecosta · 2018-09-07T22:59:06Z

mediaUpload util only allowed the uploads of a single type or all types. That makes impossible for a block to accept just the upload of images and videos.

This PR enhances the mediaUpload util and related components ( MediaUpload, MediaPlaceholder) to accept an allowedTypes prop with an array of all the supported types.

Testing

I tried to upload to each media block and verified no error occurred.
I used the Test Media Upload block available in https://gist.github.com/jorgefilipecosta/7a925ac31448832f00fceb0b0caed423, and checked I can correctly upload images and videos using the button and drag&drop. I checked that on the media library only images and videos are listed and that we can correctly select items of this types. The type string passed by MediaPlaceholder is the same for the media library and the mediaUpload function.

Required for: #9416

aduth · 2018-09-14T18:13:14Z

cc @antpb re: #9169 (similar / identical change proposed there)

aduth · 2018-09-14T18:14:06Z

packages/editor/src/utils/media-upload/media-upload.js

-		return ( allowedType === '*' ) || startsWith( fileType, `${ allowedType }/` );
+		return ( allowedType === '*' ) ||
+			some(
+				( allowedType || '' ).split( ',' ),


Why do we have allowedType as this weird string thing, when we already have sufficient types in JavaScript to represent them more semantically:

If all types are supported, why not just not pass the property rather than a '*' string ?

If multiple types are supported, why not just pass an array of mime type strings?

Hi @aduth thank you for the feedback I agree it is preferable to use an array, although this change involves a refactor and deprecations. I updated the code to use this version, I think things should work as before and we added deprecation messages for all API changes.

talldan

Hey @jorgefilipecosta. This is working really well in my tests, and it's great to have consistency with the name of the prop.

I spotted a couple of issues with the wrong prop being deprecated in a couple of places, which is important to fix.

talldan · 2018-09-27T10:52:38Z

edit-post/hooks/components/media-upload/index.js


+		let allowedTypesToUse = allowedTypes;
+		if ( ! allowedTypes && allowedType ) {
+			deprecated( 'allowedType property of wp.editor.MediaUpload', {


Looking at the diff, shouldn't the deprecated prop be type instead of allowedType?

talldan · 2018-09-27T10:55:02Z

edit-post/hooks/components/media-upload/index.js


 class MediaUpload extends Component {
-	constructor( { multiple = false, type, gallery = false, title = __( 'Select or Upload Media' ), modalClass, value } ) {
+	constructor( { allowedTypes, allowedType, multiple = false, gallery = false, title = __( 'Select or Upload Media' ), modalClass, value } ) {


It might be nice to alias the deprecated prop here, so that it's clear to users eg. type: deprecatedType.

Also feel like this line is too long now (145 chars) and should be broken onto multiple lines.

talldan · 2018-09-27T11:01:37Z

packages/editor/src/components/media-placeholder/index.js

+		const { allowedTypes, allowedType } = this.props;
+		let allowedTypesToUse = allowedTypes;
+		if ( ! allowedTypes && allowedType ) {
+			deprecated( 'allowedType property of wp.editor.MediaPlaceholder', {


Same issue here with the deprecation, the prop was type before.

talldan · 2018-09-27T11:22:57Z

packages/editor/src/utils/media-upload/media-upload.js

 * External Dependencies
 */
-import { compact, flatMap, forEach, get, has, includes, map, noop, startsWith } from 'lodash';
+import { compact, flatMap, forEach, get, has, includes, map, noop, some, startsWith } from 'lodash';


would be nice to break this onto multiple lines as well.

talldan · 2018-09-27T11:32:14Z

packages/editor/src/utils/media-upload/media-upload.js

 	// Allowed type specified by consumer
 	const isAllowedType = ( fileType ) => {
-		return ( allowedType === '*' ) || startsWith( fileType, `${ allowedType }/` );
+		return ( ! allowedTypes ) ||


This is a bit of a long line now, would be great to make this more readable, perhaps using an early return when there are no allowed types.

talldan · 2018-09-27T11:33:38Z

packages/editor/src/utils/media-upload/media-upload.js

+		return ( ! allowedTypes ) ||
+			some(
+				allowedTypes,
+				( allowedType ) => {


Alternatively this could be extracted into a separate function for checking a single allowedType.

packages/editor/src/utils/media-upload/test/media-upload.js

talldan · 2018-09-27T11:39:34Z

packages/editor/src/utils/media-upload/media-upload.js

+				( allowedType ) => {
+					// If a complete mimetype is specified verify if it matches exactly the mime type of the file.
+					if ( includes( allowedType, '/' ) ) {
+						return allowedType === fileType;


It looks like this exact match is newish functionality, so would be good to test it in a unit test

jorgefilipecosta · 2018-09-27T22:27:34Z

Thank you for the review @talldan, I think all the feedback was applied.

talldan · 2018-09-28T10:17:05Z

Looks good, I gave it a test and everything worked. 👍

I've added a commit to fix a couple of typos and a whitespace issue I just noticed.

Squashed commits: [99427d9] Changed representation to use an array. [e2bc0ab] utils: mediaUpload: Support multiple type string in mediaUpload allowedType

aduth · 2018-09-28T13:54:31Z

We should have included deprecations in the docs/deprecated.md and packages/editor/CHANGELOG.md documents.

jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Sep 7, 2018

jorgefilipecosta self-assigned this Sep 7, 2018

jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Feature] Media Anything that impacts the experience of managing media and removed [Status] In Progress Tracking issues with work in progress labels Sep 7, 2018

jorgefilipecosta changed the title ~~WIP utils: mediaUpload: Support multiple type string in mediaUpload allowedType~~ utils: mediaUpload: Support multiple type string in mediaUpload allowedType Sep 7, 2018

jorgefilipecosta force-pushed the update/mediaupload-support-allowedType-support-multiple-type-string branch from beca330 to 4a6525c Compare September 7, 2018 23:17

jorgefilipecosta mentioned this pull request Sep 10, 2018

Add "Text & Image" Block #9416

Merged

jorgefilipecosta requested a review from a team September 14, 2018 15:13

jorgefilipecosta added this to the 4.0 milestone Sep 14, 2018

aduth reviewed Sep 14, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/mediaupload-support-allowedType-support-multiple-type-string branch 5 times, most recently from 8ba6657 to 33096c5 Compare September 26, 2018 14:49

jorgefilipecosta requested a review from a team September 27, 2018 10:33

talldan requested changes Sep 27, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/mediaupload-support-allowedType-support-multiple-type-string branch 4 times, most recently from 7ec2ca1 to 093df9b Compare September 27, 2018 22:12

talldan approved these changes Sep 28, 2018

View reviewed changes

jorgefilipecosta and others added 2 commits September 28, 2018 12:04

[04044ed09] Added deprecations. (+2 squashed commits)

17ab421

Squashed commits: [99427d9] Changed representation to use an array. [e2bc0ab] utils: mediaUpload: Support multiple type string in mediaUpload allowedType

Fix whitespace issues and typos

2a840f2

jorgefilipecosta force-pushed the update/mediaupload-support-allowedType-support-multiple-type-string branch from 7b32fcf to 2a840f2 Compare September 28, 2018 11:05

jorgefilipecosta changed the title ~~utils: mediaUpload: Support multiple type string in mediaUpload allowedType~~ Media: Refactor MediaUpload, MediaPlaceholder and mediaUpload to support arrays with multiple supported types. Sep 28, 2018

jorgefilipecosta merged commit 007f15c into master Sep 28, 2018

jorgefilipecosta deleted the update/mediaupload-support-allowedType-support-multiple-type-string branch September 28, 2018 11:28

nerrad mentioned this pull request May 26, 2019

Rethink allowedType parameter in media-upload functions/components #7155

Closed

Conversation

jorgefilipecosta commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

aduth commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talldan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta commented Sep 27, 2018

Uh oh!

talldan commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth commented Sep 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jorgefilipecosta commented Sep 7, 2018 •

edited

Loading

aduth commented Sep 14, 2018 •

edited

Loading

talldan commented Sep 28, 2018 •

edited

Loading