Skip to content
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

Merged

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Sep 7, 2018

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

@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Feature] Media Anything that impacts the experience of managing media labels Sep 7, 2018
@jorgefilipecosta jorgefilipecosta added this to the 4.0 milestone Sep 14, 2018
@jorgefilipecosta jorgefilipecosta requested a review from a team September 26, 2018 14:30
title: savedMedia.title.raw,
url: savedMedia.source_url,
mediaDetails: {},
};
Copy link
Member

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

Copy link
Member Author

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!

Copy link
Member

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.

@jorgefilipecosta jorgefilipecosta force-pushed the update/pass-all-object-properties-in-media-upload branch 3 times, most recently from 6ea7a84 to 93529d6 Compare October 3, 2018 19:45
Copy link
Member

@gziolo gziolo left a 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 ) ),
Copy link
Member

@gziolo gziolo Oct 4, 2018

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.

@jorgefilipecosta jorgefilipecosta force-pushed the update/pass-all-object-properties-in-media-upload branch from 93529d6 to 5c09774 Compare October 4, 2018 21:38
@jorgefilipecosta jorgefilipecosta merged commit 638605c into master Oct 4, 2018
@jorgefilipecosta jorgefilipecosta deleted the update/pass-all-object-properties-in-media-upload branch October 4, 2018 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media Anything that impacts the experience of managing media [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants