-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Media: Refactor MediaUpload, MediaPlaceholder and mediaUpload to support arrays with multiple supported types. #9707
Conversation
beca330
to
4a6525c
Compare
return ( allowedType === '*' ) || startsWith( fileType, `${ allowedType }/` ); | ||
return ( allowedType === '*' ) || | ||
some( | ||
( allowedType || '' ).split( ',' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
8ba6657
to
33096c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -84,6 +85,19 @@ class MediaUpload extends Component { | |||
this.onClose = this.onClose.bind( this ); | |||
this.processMediaCaption = this.processMediaCaption.bind( this ); | |||
|
|||
let allowedTypesToUse = allowedTypes; | |||
if ( ! allowedTypes && allowedType ) { | |||
deprecated( 'allowedType property of wp.editor.MediaUpload', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the diff, shouldn't the deprecated prop be type
instead of allowedType
?
@@ -75,7 +76,7 @@ const getAttachmentsCollection = ( ids ) => { | |||
}; | |||
|
|||
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 } ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const { allowedTypes, allowedType } = this.props; | ||
let allowedTypesToUse = allowedTypes; | ||
if ( ! allowedTypes && allowedType ) { | ||
deprecated( 'allowedType property of wp.editor.MediaPlaceholder', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue here with the deprecation, the prop was type
before.
@@ -1,7 +1,7 @@ | |||
/** | |||
* 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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to break this onto multiple lines as well.
@@ -68,11 +68,22 @@ export function mediaUpload( { | |||
|
|||
// Allowed type specified by consumer | |||
const isAllowedType = ( fileType ) => { | |||
return ( allowedType === '*' ) || startsWith( fileType, `${ allowedType }/` ); | |||
return ( ! allowedTypes ) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return ( ! allowedTypes ) || | ||
some( | ||
allowedTypes, | ||
( allowedType ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively this could be extracted into a separate function for checking a single allowedType.
( allowedType ) => { | ||
// If a complete mimetype is specified verify if it matches exactly the mime type of the file. | ||
if ( includes( allowedType, '/' ) ) { | ||
return allowedType === fileType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this exact match is newish functionality, so would be good to test it in a unit test
7ec2ca1
to
093df9b
Compare
Thank you for the review @talldan, I think all the feedback was applied. |
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. |
7b32fcf
to
2a840f2
Compare
We should have included deprecations in the |
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