MediaPlaceholder multiple and gallery properties #9139
Conversation
The `gallery` property should be set to `true` only after checking that the selected `type` of media is not `audio`.
LuigiPulcini
changed the title
| @@ -127,7 +127,7 @@ class MediaPlaceholder extends Component { | |||
| { __( 'Upload' ) } | |||
| </FormFileUpload> | |||
| <MediaUpload | |||
| gallery={ multiple } | |||
| gallery={ multiple && type != "audio" } | |||
There was a problem hiding this comment.
I'd say, it's probably better if the check is multiple && type === 'image'
There was a problem hiding this comment.
Yes, I agree.
And, actually, there should be two different lines, one for gallery and one for playlist, like this.
gallery={ multiple && type === 'image' }
playlist={ multiple && type === 'audio' }
gallery and playlist check for multiple and type, being gallery true when multiple is true and type is image, while playlist true when multiple is true and type is audio.
| @@ -127,7 +127,8 @@ class MediaPlaceholder extends Component { | |||
| { __( 'Upload' ) } | |||
| </FormFileUpload> | |||
| <MediaUpload | |||
| gallery={ multiple } | |||
| gallery={ multiple && type === "image" } | |||
| playlist={ multiple && type === "audio" } | |||
There was a problem hiding this comment.
I don't think we support playlists yet in MediaUpload and even if we did, there are playlists of videos.
I think it's probably better to remove until #9169
There was a problem hiding this comment.
I hear what you are saying regarding the video playlists and that could be easily changed to something accommodating for both audio and video types.
But I have to disagree regarding the possibility to wait until #9169: analyzing that code – particularly this part of the edit.js file – it is clear that it simply tried to circumvent the current MediaPlaceholder limitation, recreating the whole component inside the edit.js file, only because the current MediaPlaceholder implementation does not allow to manage media types other than images.
So, I believe it is important to fix the MediaPlaceholder before refactoring the playlist block, which will allow a more streamlined playlist component as well.
There was a problem hiding this comment.
The underlying MediaUpload component doesn't support the playlist yet. So why to pass a useless prop?
There was a problem hiding this comment.
You are right.
I edited the proposed change accordingly.
My point is the MediaPlaceholder should be fixed before refactoring the playlist because of the reasons I explained above.
There was a problem hiding this comment.
I agree it would be great if the MediaUpload also supports playlists out of the box.
The `gallery` property is set to `true` when `multiple` is `true` and `type === "image"`
| @@ -127,7 +127,7 @@ class MediaPlaceholder extends Component { | |||
| { __( 'Upload' ) } | |||
| </FormFileUpload> | |||
| <MediaUpload | |||
| gallery={ multiple } | |||
| gallery={ multiple && type === "image" } | |||
There was a problem hiding this comment.
Looks like there's an eslint issue here. We should use single quotes. You can test locally using npm run lint or install an eslint plugin for your IDE.
changed type check using single quotes instead of double quotes
The
galleryproperty should be set totrueonly after checking that the selectedtypeof media is notaudio.Description
The MediaPlaceholder component can be used to select multiple files also with audio files. In addition to being counterintuitive, forcing
galleryto betrueonly based on themultipleproperty does not allow to work with audio files. In fact, whentypeis set to"audio"andmultipleistrue, the MediaUpload component only shows imagesAdditional Information
In addition to the proposed change, it could be possible to add another line as follows:
This could be done when a playlist block is finally added to Gutenberg.
Checklist: