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

Always prefer "large" image size #11682

Merged
merged 6 commits into from Nov 15, 2018

Conversation

Projects
None yet
5 participants
@azaozz
Contributor

azaozz commented Nov 9, 2018

Fix to use the url to the "large" size when inserting images in both galleries and image blocks.
(Was wondering why there are two copiers of pickRelevantMediaFiles())?

See #11377 and #11377 (comment).

Always prefer "large" image size
For galleries and image blocks.

@azaozz azaozz requested review from noisysocks, iseulde and youknowriad Nov 9, 2018

@Luehrsen

This comment has been minimized.

Contributor

Luehrsen commented Nov 11, 2018

As I was working on something similar recently, please consider that there is often not a 'large' variant, when the image is too small. So there should be a fallback to 'full', when 'large' does not exist.

For reference: This is what I use at the moment

/**
 * Get the largest accaptable available size from an wp image object
 *
 * @param  {Object} image              The WordPress image object
 * @param  {Array}  acceptableSizes    The array of acceptable images, from least to most acceptable
 *
 * @return {Object}                    The object of the image size with details of the size.
 */
export const getLargestAvailableSize = ( image, acceptableSizes = [ 'full', 'large' ] ) => {
	let imageSize = false;
	const sizes = getAvailableSizes( image );

	if( isEmpty( sizes ) ) {
		return false;
	}

	map(acceptableSizes, (size) => {
		if( ! isEmpty( sizes[ size ] ) ) {
			imageSize = sizes[ size ];
			imageSize.size = size;
		}
	});

	return imageSize;
}

/**
 * Get the available sizes from a wp image object
 *
 * @param  {Object} image  The WordPress image object
 *
 * @return {Object}        An object with the available sizes
 */
export const getAvailableSizes = ( image ) => {

	let sizes = get( image, [ 'media_details', 'sizes' ], {} );

	// REST API and media frame are inconsistent, so we have a second place to look at
	if( isEmpty(sizes) ) {
		sizes = get( image, [ 'sizes' ], {} );
	}

	return sizes;
}
@azaozz

This comment has been minimized.

Contributor

azaozz commented Nov 11, 2018

please consider that there is often not a 'large' variant...

Of course. That case is common and is handled in both this PR and the (original) #11377. See https://github.com/WordPress/gutenberg/pull/11682/files#diff-34f83402fe8954f15f8d404d01c9e4c9R49 where it tries to get the "large" size and if it doesn't exist falls back to the full size. (I know it's not so easy to read/understand what's going on in that chunk of core, will add a comment to explain better).

@youknowriad youknowriad added this to the 4.4 milestone Nov 12, 2018

@youknowriad youknowriad added the Media label Nov 12, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 12, 2018

What about other media blocks (corer, media and text), do they need this change as well?

@azaozz

This comment has been minimized.

Contributor

azaozz commented Nov 12, 2018

@youknowriad yes, definitely. Both the "cover" and "media & text" blocks would need something like this. Was just wondering what's up with the duplicate pickRelevantMediaFiles() and should we be trying to use it in these two blocks, or make a "global" function for use everywhere.

@azaozz

This comment has been minimized.

Contributor

azaozz commented Nov 12, 2018

Updated this PR to also try to use the "large" file in the "cover" and "media and text" blocks.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 12, 2018

what's up with the duplicate pickRelevantMediaFiles() and should we be trying to use it in these two blocks, or make a "global" function for use everywhere

@azaozz Yes, it definitely feels like something can be extracted but at the same time, the properties/attributes we keep are very much dependent on the block (attributes) which means it can be specific.

@azaozz

This comment has been minimized.

Contributor

azaozz commented Nov 12, 2018

Yeah, it is just one line of code that looks for the "large" size in the image props passed by either the media modal (on inserting an image) or the API, as they are different/not normalized.

It could be helpful for plugins adding blocks containing images, but then they can just copy whatever is used in the default blocks :)

@youknowriad youknowriad requested a review from jorgefilipecosta Nov 13, 2018

@jorgefilipecosta

This PR worked well in my tests for both direct uploads and selections from the media library 👍
I agree with this changes for the Image block, given that the block allows the user to select a different size, e.g.: full.
The cover, media & text, and gallery blocks don't allow the user to pick a different size.
On the gallery block probably selecting a smaller image size is also ok given that perhaps the images shown on the block are not big anyway.

For the cover and Media & Text block, the case is complex. These blocks provide a kind of layout; the user may prepare an image with a specific size on purpose to use on the cover block, then uploads the image and always gets an image with a smaller size than what was uploaded without an option to use the original size.

In my perspective, these two blocks are blocks where there is more significant expectation that the user gets what he uploads, e.g., the user can even select videos where problems with size/data transferred can be more prominent.

I think we can advance with the changes for image and gallery right now and rethink the changes in "Media & Text" and Cover when these blocks have a size picker. When applying the changes for this blocks, I think we need some rule, e.g., only use the large size if its width is greater than the block width or something like that, in my testing sometimes images on large size are still very small to use on this blocks.

@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 15, 2018

I think we can advance with the changes for image and gallery right now and rethink the changes in "Media & Text" and Cover when these blocks have a size picker.

That's a good point, do you think we can do that @azaozz ?

@azaozz

This comment has been minimized.

Contributor

azaozz commented Nov 15, 2018

@youknowriad sure, can revert the PR to only change sizes for the image and gallery blocks. However thinking we should use "large" for the Media and Text block too.

... the user may prepare an image with a specific size on purpose to use on the cover block, then uploads the image and always gets an image with a smaller size than what was uploaded without an option to use the original size.

Right, this is the second most-common case :) The top one is that the user would upload a photo they have taken (or select one from the media library). Then the "full" image size would usually be quite large, often greater than 3-4MB, and not "optimized for the web".

For the Cover image block, best way to "fix" that is to add another larger size generated by default in WP. Something like 1600px or even 2000px. We've been discussing this for a while in core's #media slack channel. There are some technical issues but hoping it will happen soon.

As that is not implemented yet, maybe we should keep using the "full" size despite the downsides. Then add more image meta (block props) to it so themes can choose to use a smaller image when appropriate. No website should be forcing its visitors to download huge, non-optimized image files! :)

The other option would be to let the user select the size in the editor, perhaps limiting that to only "large" and "full". Thinking we probably can try that, it would be a really nice enhancement.

For the Media and Text block thinking the image should default to "large". This block typically sets the media to 50% wide so using a "full" size image there wouldn't make sense in most cases. (Keep in mind that when an uploaded image is not large enough to create the "large" size, we automatically fall back to using the "full" size).

Also we will need to make sure images used in this block get the srcset and sizes attributes on the front-end. Then the actual image size used in the editor won't matter much.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 15, 2018

I'm interested in moving this forward for whatever you think is best. I'd be fine with consistency (size picker and large by default for all) as this makes the expectations similar across blocks. But I'm fine shipping whatever you think is best for now.

@azaozz

This comment has been minimized.

Contributor

azaozz commented Nov 15, 2018

OK, let me update the PR, give me few min :)

@azaozz

This comment has been minimized.

Contributor

azaozz commented Nov 15, 2018

@youknowriad updated the pr, reverted to using the "full" image size in the Cover block, and re-tested. Seems good for merging. Would love it if we get the chance to add the image file/size picker to the Cover block :)

@youknowriad

LGTM 👍

@youknowriad youknowriad modified the milestones: 4.5, 4.4 Nov 15, 2018

@azaozz azaozz merged commit e583606 into master Nov 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@azaozz azaozz deleted the fix/prefer-image-size-large branch Nov 15, 2018

@joemcgill

This comment has been minimized.

Contributor

joemcgill commented Nov 15, 2018

Tested post-merge and this seems to be working and addresses one of the main issues outlined in this comment to address #6177.

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Always prefer "large" image size (WordPress#11682)
Prefer "large" image size (when available) for Gallery, Image and Media and Text blocks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment