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

Include all image sizes on the media upload object (if they exist) #7605

Merged
merged 4 commits into from Jul 3, 2018

Conversation

Projects
None yet
4 participants
@danielbachhuber
Member

danielbachhuber commented Jun 28, 2018

image

Fixes #7483

@danielbachhuber danielbachhuber requested a review from WordPress/gutenberg-core Jun 28, 2018

@danielbachhuber danielbachhuber added this to the 3.2 milestone Jun 28, 2018

@dseidl

dseidl approved these changes Jun 28, 2018

works for me - thanks!

@@ -119,6 +119,7 @@ export function mediaUpload( {
id: savedMedia.id,
link: savedMedia.link,
url: savedMedia.source_url,
sizes: get( savedMedia, [ 'media_details', 'sizes' ], {} ),

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 28, 2018

Member

Can we just leave the value undefined when sizes are not available? sizes: get( savedMedia, [ 'media_details', 'sizes' ] ). I think we don't need to return an empty object when sizes are not relevant.

This issue makes me question if mediaUpload should not just return the objects it receives from the server

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 28, 2018

Member

This issue makes me question if mediaUpload should not just return the objects it receives from the server

Yes, I'm curious whether there was a deliberate decision to limit the object size. My preference would be to return the entire object (particularly because different media types have different attributes)

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 28, 2018

Member

My preference would be to return the entire object (particularly because different media types have different attributes)

Although, if we do this now, we change the shape of the data returned to the callback.

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 28, 2018

Member

Another option: create savedMedia.mediaDetails = {}, and the only include a sizes attribute for images with sizes. I think this is my preferred middle ground.

This comment has been minimized.

@danielbachhuber

@danielbachhuber danielbachhuber requested a review from WordPress/gutenberg-core Jul 3, 2018

@@ -115,14 +115,18 @@ export function mediaUpload( {
return createMediaFromFile( mediaFile, additionalData ).then(
( savedMedia ) => {
const mediaObject = {
let mediaObject = {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jul 3, 2018

Member

I think we can still use const, we are not changing the mediaObject reference we are just changing a key in the object.

This comment has been minimized.

@danielbachhuber

danielbachhuber Jul 3, 2018

Member

I think we can still use const, we are not changing the mediaObject reference we are just changing a key in the object.

This is bizarre and seems like a bug. Constant means constant, or unchanging. Reassignment, even to some subset of the data, isn't permitted in PHP.

I've made the change regardless 4f3acb7

@danielbachhuber danielbachhuber requested a review from WordPress/gutenberg-core Jul 3, 2018

@jorgefilipecosta

This seems to work correctly thank you for iterating on this @danielbachhuber 👍

@danielbachhuber danielbachhuber merged commit 30f25b9 into master Jul 3, 2018

2 checks passed

codecov/project 46.76% (-0.01%) compared to 1b2db4e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber deleted the 7483-image-sizes branch Jul 3, 2018

@cfaria

This comment has been minimized.

cfaria commented Nov 14, 2018

Correct me if I'm wrong but I can't see the utils folder anymore on current version 4.3, so this merge by @danielbachhuber is not there.

Hope to get those custom image sizes back.

Maybe I'm doing it wrong and I should get custom sizes with another object?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment