-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
utils: mediaUpload: Pass all available properties in the media object #9704
utils: mediaUpload: Pass all available properties in the media object #9704
Conversation
title: savedMedia.title.raw, | ||
url: savedMedia.source_url, | ||
mediaDetails: {}, | ||
}; |
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.
Please note that there is this subtle difference between media_details
and mediaDetails
in both objects. In addition, we no longer normalize mediaDetails.sizes
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.
Nice catch @gziolo. We are not using mediaDetails anywhere in our code but a plugin may be using it.
I feel we should use what is returned by the rest API, so we are consistent with media_details used in other rest API responses. I added a deprecation so we can keep the previous version for some versions.
Thank you for your review!
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.
Can we auto-map keys instead? We should really stop adding deprecations at this stage.
6ea7a84
to
93529d6
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.
It works well in my testing. I left a comment which offers another refactoring for picking properties, to remove a few keystrokes. It's not a blocker, take your own judgment.
@@ -87,7 +89,7 @@ class GalleryEdit extends Component { | |||
|
|||
onSelectImages( images ) { | |||
this.props.setAttributes( { | |||
images: images.map( ( image ) => pick( image, [ 'alt', 'caption', 'id', 'link', 'url' ] ) ), | |||
images: images.map( ( image ) => pick( image, RELEVANT_MEDIA_FIELDS ) ), |
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.
You could export a method out of ( image ) => pick( image, RELEVANT_MEDIA_FIELDS )
:
export const pickRelevantMediaFiles = ( image ) => pick( image, RELEVANT_MEDIA_FIELDS );
and also use it inside this file.
93529d6
to
5c09774
Compare
Description
Initially, mediaUpload util only passed to the callback function of the invoker a simple object with the URL of the file, a caption, and an alt. With time needs for other props appeared and the function was changed to add the missing props. The most recent need was mediaDetails.sizes.
Now with the most recent updates in #9416 I discovered I need yet another property the media type.
I thought a little bit and I think the function should return all the available properties. Omitting props will limit the things blocks can do and will probably force them to use their own media upload if they really need a prop we are omitting.
How has this been tested?
I checked that blocks where we upload files (image, gallery, video, audio and file) the uploads still work as before.
Required for: #9416