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

Add more data about the image as block props in the image block #11973

Closed
wants to merge 45 commits into from

Conversation

@azaozz
Copy link
Contributor

commented Nov 16, 2018

This is the second "chunk" split from #11377. It contains:

  1. Adds fileWidth, fileHeight, editWidth, userSet block attributes. These would allow us to add width and height, and the themes to generate better sizes attributes on the front-end.
    • fileWidth and fileHeight are the image file dimensions at the time when the image was inserted. For "local" images they can also be "extracted" from the image meta, however there are edge cases where that meta may have changed since the post was published. Also needed for non-resized external images where we don't know the width and height.
    • editWidth holds the block width inside the editor at the moment the post was created/edited. Can be used to scale images on the front-end when the block widths don't match (in themes with fluid width or after changing the theme).
    • userSet indicated when the user has set the width and height manually. In these cases the theme should (probably) honor the settings when scaling.
  2. Fixes the 25, 50, 75, 100% buttons to scale the image relative to the block width. This makes it "visual". Works well in combination with editWidth when scaling images on the front-end. Also images "line-up" when setting them to the same scale.
  3. Fixes resizing by dragging to prevent upscaling.
  4. Fixes direct input resizing to display values only when they were entered by the user.
  5. Makes the image block non-dynamic (compared to #11377) and uses the new render_block filter to add attributes to the img tag on the front-end, including srcset and sizes.

@azaozz azaozz requested review from aduth, jasmussen and joemcgill Nov 16, 2018

@azaozz azaozz added this to the WordPress 5.0 milestone Nov 16, 2018

@mor10

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

I'm trying to test this but am having no luck. Probably doing something stupid or wrong:

function twentynineteen_calculate_image_sizes_attr( $image_attributes, $block_attributes ) {
	var_dump($image_attributes);
	var_dump($block_attributes);
}

add_filter( 'render_block_core_image_tag_attributes', 'twentynineteen_calculate_image_sizes_attr', 10, 2 );

This is just a rough test to see what is contained within the attributes so I can build my conditions. Running the above code in WP 4.9.8 and this fork does nothing. Various permutations have not resulted in anything happening when calling the render_block_core_image_tag_attributes filter.

Like I said, probably doing something wrong. Please advise.

@azaozz

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

Uh, sorry, should have explained better. All the code in lib/compat.php (the render_block_core_image_tag_attributes filter is there) runs off the new render_block filter here. That filter is in the 5.0 branch, was added ~ a week ago. Please test in recent 5.0-beta :)

Also, it is not there yet but the (old) wp_calculate_image_sizes filter will get the $block_attributes too. Let me know if something else/more is needed, also for the attributes passed from the editor.

@mor10

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2018

@azaozz What, in your opinion, should be the preferred method for themes to control the sizes attribute? The wp_calculate_image_sizes filter or the render_block_core_image_tag_attributes filter? Is one more robust / performant / future proof than the other?

@mor10

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2018

@azaozz This works! Look at the PR to Twenty Nineteen above.

@azaozz

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2018

@mor10 these filters have different scopes, depends what the theme wants to accomplish :)

  • The new render_block_core_image_tag_attributes gives full control of the <img> tag on the front-end (after this PR is merged of course). The theme should use this filter if it needs to set more/other attributes, or to re-calculate some of the existing attributes except width, height, srcset, and sizes. These four have their own filters that (will) get the same data, the existing filters for srcset and sizes plus the new block_core_image_get_width_height. Another difference is that render_block_core_image_tag_attributes runs only in WP 5.0+ and only when rendering the image block. In the future it may run for other blocks that contain images (Cover, Media and Text), or we may add other filters there..?

  • The wp_calculate_image_sizes is preferred when the theme looks at the sizes attribute. It will get the $block_attributes once this PR is merged, which will (hopefully) be enough to generate a much more precise sizes attribute. It also exists in older WP versions making it more compatible.

Ideally every theme that supports the Block editor would hook into block_core_image_get_width_height (as it controls image scaling and to some extend generation of srcset and sizes) and wp_calculate_image_sizes. These two would give "full control" over sizing of images on the front-end. Alternatively it can use render_block_core_image_tag_attributes and re-calculate some of the img tag attributes (it runs "after the fact" so it will be a tiny bit slower).

Thinking we will have to post something like this explanation on /make as soon as this PR is merged. Would you be up for that? :)

Edit: just renamed gutenberg_get_image_width_height filter to block_core_image_get_width_height for consistency. Fixed above.

@mor10

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2018

I can help write a post explaining all this. Will test the different approaches first thing next week to get a clear picture of what each of the filters does and how best to guide theme developers to get this baked in before launch.

@aduth

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

  1. Adds fileWidth, fileHeight, editWidth, userSet block attributes. These would allow us to add width and height, and the themes to generate better sizes attributes on the front-end.

I have a hard time imagining there's not at least some redundancy in having all of width, fileWidth (fileHeight), userSet, and editWidth.

  • Can userSet be derived as a function of the presence of width or height?
  • When would editWidth not be equal to width in scenarios where e.g. it'd be used for scaling?
  • Does it matter what the fileWidth and fileHeight are for external images? Can that be determined server-side (cached if necessary) if it's relevant there?
    • At worst, could it be reflected in width and height?

@mtias mtias modified the milestones: WordPress 5.0, 4.5 Nov 19, 2018

@azaozz

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

Of course there is some redundancy, but it's a "good thing" imho :)

Can userSet be derived as a function of the presence of width or height?

Sure, except when were width and height set from resizing by dragging, and when were they set directly by the user. In some cases that may not matter in others (scaling on the front end) it's pretty critical.

When would editWidth not be equal to width in scenarios where e.g. it'd be used for scaling?

Pretty much all resized images? Both "100% width" and resized images can potentially be scaled.

Does it matter what the fileWidth and fileHeight are for external images?

Yes, since the image block doesn't add width/height for non-resized images. There must be some way for the theme (front-end) to get at least the actual image dimensions. If these are known, the theme (or core) can decide to scale these images too. Also, images outputted without width and height attributes behave "badly", make the page "jump" while loading, push the comments form below the fold (while the user is trying to type), etc.

Can that be determined server-side (cached if necessary) if it's relevant there?

Possibly, but to store it we will have to change the image meta schema.

At worst, could it be reflected in width and height?

Sure. That would mean the image block will always add width and height attributes to all images? That behavior seems to be the default in pretty much all editors I've seen, not sure why this was removed from the image block...

However, even when having width and height we would still prefer to know the fileWidth and fileHeight at least for external images. And, if we add them for external, why not for internal? :)

I don't see any problem in saving some extra/redundant image meta in the image block props. This will make in more futureproof. Not having enough meta is much worse (see the image attachments meta where we could have stored few more very very useful values).

this.props.setAttributes( { url, width: undefined, height: undefined } );
setWidthHeight( width, height, fileWidth, fileHeight, userSet ) {
// Set image size and also whether set directly by the user.
userSet = userSet || undefined;

This comment has been minimized.

Copy link
@mtias

mtias Nov 19, 2018

Contributor

Is this needed?

This comment has been minimized.

Copy link
@dmsnell

dmsnell Nov 19, 2018

Contributor

if this is what I'm thinking it is, it sounds like it's something like intentionallySetWidth and intentionallySetHeight or displayWidth and displayHeight

is there confusion since this boolean value doesn't entirely describe why it exists or what the user set?

This comment has been minimized.

Copy link
@azaozz

azaozz Nov 20, 2018

Author Contributor

You mean userSet = userSet || undefined; or userSet? The first is probably not, just some "normalization", but as that's an internal function it can be skipped.

The userSet indicates whether the image width and height were set directly by the user (typed in the width or height fields in the inspector), or they were added as a result of resizing the image by dragging. This would make a difference when the theme or a plugin is deciding to scale or not to scale an image on the front-end.

In the previous iteration these were actually two values: userWidth and userHeight. Switched it to one boolean after some critique that we store too much image meta in the block props. Can revert that easily :)

This comment has been minimized.

Copy link
@aduth

aduth Nov 20, 2018

Member

Worth noting there's a subtlety here in that if the argument was called with userSet = false, it would normalize the value to undefined, which in practice has the same result since the default of the attribute is false. And while that default is unlikely to change, if it were to, the normalizing would have the adverse effect of preventing a value of explicit false to be provided as an argument.

}
return $content;
}

This comment has been minimized.

Copy link
@dmsnell

dmsnell Nov 19, 2018

Contributor

is there a reason for this not to be hooking into the block filter so that we can get the actual attributes?

function _gutenberg_cache_images_meta( $html, $block ) {
	if ( 'core/image' === $block[ 'blockName' ] ) {
		_prime_post_caches( $block[ 'attrs' ][ 'id' ], /* why aren't these */ false, /* commented? */ true );
	}

	return $html;
}

add_filter( 'rendered_block', '_gutenberg_cache_images_meta', 3 );

This comment has been minimized.

Copy link
@azaozz

azaozz Nov 20, 2018

Author Contributor

The purpose here is to get the IDs of all images (where we will do srcset and sizes) from the post before we parse the blocks. That's needed to get all image attachment meta in one go from the DB as that's faster.

So we need an array with all attachment IDs for the images used in the post. This is equivalent to https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/media.php#L1336.

That should also include images from "Cover" and "Media and Text" blocks, and probably the Gallery block (if images there are "hardcoded"). Any ideas how to do that without a regex are very welcome :)

This comment has been minimized.

Copy link
@aduth

aduth Nov 20, 2018

Member

Previously: #11377 (comment)

I'm not sure I understand why this would need to occur prior to the parse. If it at least occurs prior to any block evaluating its render_callback, wouldn't that be equally sufficient to warm the cache?

This comment has been minimized.

Copy link
@aduth

aduth Nov 20, 2018

Member

If it at least occurs prior to any block evaluating its render_callback, wouldn't that be equally sufficient to warm the cache?

Since this doesn't exist as an option today anyways (requires filter on this value), we probably can't do anything of this sort right now.

Should follow-up with a Trac ticket as appropriate.

This comment has been minimized.

Copy link
@azaozz

azaozz Nov 20, 2018

Author Contributor

Yeah, this doesn't do anything if we do it "per block". It makes sense only when it happens for all of post_content at once before we start to look at individual images and generating srcset and sizes.

This comment has been minimized.

Copy link
@dmsnell

dmsnell Nov 20, 2018

Contributor

if we hook into the block-level filter (the design of which I favor in #10108) then we can track the parse through the blocks and then prime the cache on the do_blocks filter which occurs after parsing and rendering is complete.

that is, we could imagine something like this…


function _gutenberg_cache_images_meta( $html, $block ) {
	if ( $block['blockName'] === 'core/image' ) {
		$this->add_image( $block['attrs']['id'] );
	}

	return $html;
}
add_filter( 'rendered_block', '_gutenberg_cache_images_meta', 3 );

function _gutenberg_prime_cache() {
	if ( empty( $this->images ) ) {
		return;
	}

	// whatever command does all images in one go
	_prime_post_caches( $this->images, false, true, true, true, false, false );
}
add_filter( 'do_blocks', '_gutenberg_prime_cache' );

and this because we know that the final do_blocks filter runs after block-level processing.

This comment has been minimized.

Copy link
@dmsnell

dmsnell Nov 26, 2018

Contributor

noting that this is still based on a RegExp instead of using the per-block and per-document filters

This comment has been minimized.

Copy link
@azaozz

azaozz Nov 27, 2018

Author Contributor

Another option would be to "unify" this with the "old" cache priming. We have the (required) wp-image-#### className on all images, including in Cover and Media and Text blocks.

Will need to abstract wp_make_content_images_responsive() a bit, move the caching out and run it earlier on the_content filter.

On the other hand _prime_post_caches() is "clever enough" to not fetch posts and meta that are already cached so running two pre-caching methods won't "hurt" anything.

This comment has been minimized.

Copy link
@mtias

mtias Nov 28, 2018

Contributor

I'm leaning towards doing this in conjunction with #10108 because I want to be sure the design is ergonomic enough to power this use case in a way that is scalable and transitive to other cases on other blocks.

}

updateImageURL( url ) {
this.props.setAttributes( { url, width: undefined, height: undefined } );
setWidthHeight( width, height, fileWidth, fileHeight, userSet ) {

This comment has been minimized.

Copy link
@dmsnell

dmsnell Nov 19, 2018

Contributor

this name is definitely confusing here…does the width have a height?

This comment has been minimized.

Copy link
@azaozz

azaozz Nov 20, 2018

Author Contributor

Well, you cannot really read the name of a function without reading it's parameter names, then it becomes pretty clear imho :) But lets change it to something more descriptive.

width: undefined,
height: undefined,
userSet: undefined,
} );

This comment has been minimized.

Copy link
@dmsnell

dmsnell Nov 19, 2018

Contributor

there's no need here for two calls to setAttributes() - we can collapse them into one in a couple of different ways. my favorite is with Object.assign()

// what does this name mean?
resetWidthHeight( fileWidth, fileHeight ) {
	this.props.setAttributes( Object.assign(
		{
			width: undefined,
			height: undefined,
			userSet: undefined,
		},
		fileWidth && fileHeight && {
			fileWidth,
			fileHeight,
		}
	) );
}

This comment has been minimized.

Copy link
@azaozz

azaozz Nov 20, 2018

Author Contributor

Sure. The purpose here is to make it as easy to read and understand as possible. After going through babel and minification, the code that actually runs in production will be completely different.

(I actually "have a beef" with the way a lot of the code is written. All the redundant indentation, nested lambda functions (no names = poor context), "clever" conditionals, almost no inline comments... There seem to be quite a few cases where the code becomes more readable after running it through babel and minification, then "unminifying" it. But lets look at that after 5.0 is out.) :)

This comment has been minimized.

Copy link
@aduth

aduth Nov 20, 2018

Member

there's no need here for two calls to setAttributes() - we can collapse them into one in a couple of different ways. my favorite is with Object.assign()

(Noting that the supplied example doesn't use Object.assign and wouldn't work as written)

This comment has been minimized.

Copy link
@dmsnell

dmsnell Nov 20, 2018

Contributor

(Noting that the supplied example doesn't use Object.assign and wouldn't work as written)

Good call! 😆 (fixed)

This comment has been minimized.

Copy link
@aduth

aduth Nov 20, 2018

Member

Sure. The purpose here is to make it as easy to read and understand as possible. After going through babel and minification, the code that actually runs in production will be completely different.

The main thing for me here would be to avoid multiple calls to setAttributes, as it will incur a render for each (unnecessary impact on performance). This will remain true after transpilation and minification.

I pushed bf67408 which I think is a reasonable compromise to preserve a similar readability.


return {
width: width,
height: height,

This comment has been minimized.

Copy link
@dmsnell

dmsnell Nov 19, 2018

Contributor

looks like we're introducing rounding errors when none are needed.

const width = targetWidth = ( fullWidth * ( targetWidth / fullWidth ) );

if we look in the math world the fullWidths will cancel each other out, but in JS we'll probably convert first to the ratio and then multiply, leaving error. the good news is that the accurate way is faster due to needing no computation.

constrainImageDimensions( width, height, targetWidth ) {
	return {
		height: Math.max( 1, Math.round( height * targetWidth / width ) ),
		width,
	};
}

Also, since this doesn't depend on any instance variables, might as well put it outside of the class and make it a simple helper function?

function scaledBy( value, ratio ) {
  return Math.max( 1, Math.round( value * ratio ) );
}

…

const width = fullWidth;
const height = scaledBy( height, width / targetWidth );

This comment has been minimized.

Copy link
@azaozz

azaozz Nov 20, 2018

Author Contributor

constrainImageDimensions is pretty much a copy of core's wp_constrain_dimensions(): https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/media.php#L400. We can change/improve it as long as it works in exactly the same way (and is "easy to read") :)

since this doesn't depend on any instance variables, might as well put it outside of the class and make it a simple helper function

Sure, it probably can be re-used somewhere else.

This comment has been minimized.

Copy link
@dmsnell

dmsnell Nov 20, 2018

Contributor

We can change/improve it as long as it works in exactly the same way

well it caught my attention because a bug in the algorithm is already mentioned in the comment 😉
then I realized it's doing much more than it needs to, and unlike the core function we're only scaling one dimension.

seems silly to me to drag out old code that fulfills other purposes in a way that's inefficient and buggy 🙃 (my tone is light-hearted here).

"easy to read"

probably won't ever reach consensus here, but I am sharing my own difficult in figuring out what the code is supposed to do. you can take what you want from it. my bias is that I'm pretty ignorant and unable to keep lots of stuff in my head at once; smarter people will probably be able to do more in-head parsing/compiling/state-tracking.

fileWidth = imageData.width;
fileHeight = imageData.height;
return false;
}

This comment has been minimized.

Copy link
@dmsnell

dmsnell Nov 20, 2018

Contributor

using map without returning a new list and when modifying things inline is pretty unusual. we have a couple of choices depending on what our intention is. it looks like we're looking for a specific value and using that if it exists.

updateImageURL( url, sizeOptions ) {
	const { width, height } = sizeOptions;

	if ( width && height ) {
		return this.setAttributes( {
			url,
			fileWidth: width,
			fileHeight: height,
		} );
	}

	const option = find( sizeOptions, ( { value } ) => value === url );

	if ( option ) {
		return this.setAttributes( {
			url,
			fileWidth: option.imageData.width,
			fileHeight: option.imageData.height;
		} );
	}
}

This comment has been minimized.

Copy link
@azaozz

azaozz Nov 20, 2018

Author Contributor

Right, it doesn't need to use map there. In the previous iteration sizeOptions was different so we always needed to iterate to find the proper values, now it's simpler.

@azaozz

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

@mor10 correct. Ideally this will be merged together with a way for the theme to set uniform block_width for both the editor and the front-end (other than CSS).

@markfinst

This comment has been minimized.

Copy link

commented Nov 24, 2018

What about adding srcset by default?

  • Every image within Image or Gallery block has srcset and sizes.
  • srcset contains list of all available image sizes which aspect ratio matches original. If user crops image - we return all images that match aspect ratio of cropped.
  • sizes attribute is generated automatically depending on where image is added (for example, if image is added to full-width container sizes would be 100vw).

To implement this, WordPress core badly needs an API method for srcset and sizes, specifically:

get_image_srcset_attribute_array( $image_id, $max_image_width );

Returns array of images that should be be used in srcset, in format:

array(
  '123' => array(
    'url' => 'https://...',
    'width' => 600,
    'height' => 400,
    'size_key` => 'thumbnail'
  ),
 ...
)

So theme developers can easily filter it.

get_image_sizes_attribute( $size_key, $size_ratio = 1 )

  • $size_key is a key that describes location of the image (full-width, align-left, content ...)
  • $size_ratio is relative width multiplier, for example if image is added in 1/3 column, size_ratio should be 0.33.

Should return sizes attribute value.

azaozz added some commits Nov 24, 2018

Check only for valid HTML 5.0 attribute names
All HTML attribute names match `/[a-z0-9-]+/` except `data-*`. Fix the check to do the same.
Update handling of "wide" and "full" alignment for the front-end
Update selecting a larger image when alignment is "wide" or "full". Look for larger sizes that may have been added by themes or plugins, then fall back to the original image (if larger). Keep the arbitrary limit of 4300px to avoid forcing site visitors to download huge image files.

const measure = document.createElement( 'div' );
measure.className = 'wp-block editor-block-list__block';
layout.appendChild( measure );

This comment has been minimized.

Copy link
@azaozz

azaozz Nov 24, 2018

Author Contributor

To avoid the arbitrary clientWidth - 30, perhaps can append another div here with class="editor-block-list__block-edit" to mimic the default block layout, then get clientWidth from it. I'll try it.

I know, it's fragile, but seems workable for now.

Done in 078a75b.

There still appear to be an inconsistency/delay when TwentyNineteen is active, and a post with images is opened for editing. Seems caused by the CSS as the images are also not sized properly, "hang out on the right" a bit. Looks like some CSS is loaded/added later, after rendering has finished. That causes a re-flow in the browser. When that is fixed, the clientWidth will be correct from the start.

azaozz added some commits Nov 24, 2018

*/
function gutenberg_render_block_core_image( $html, $block ) {
// Old post or... something's wrong.
if ( empty( $html ) || empty( $block['attrs'] ) ) {

This comment has been minimized.

Copy link
@aduth

aduth Nov 27, 2018

Member

I think we should be shortcutting here if the block is not the image block? It'll eventually be shortcutted at the below test of empty( $block_attributes['url'] ), but it seems both indirect and wasteful to let us get that far, when this is called for every block.

This comment has been minimized.

Copy link
@azaozz

azaozz Nov 28, 2018

Author Contributor

Right, good catch. Added in 648811f.

Update `gutenberg_render_block_core_image()`
Return early when the block is not `core/image`.
$image_attributes = apply_filters( 'render_block_core_image_tag_attributes', $image_attributes, $block_attributes, $html );
$attr = '';
foreach ( $image_attributes as $name => $value ) {

This comment has been minimized.

Copy link
@mtias

mtias Nov 28, 2018

Contributor

I'm not super confident in going this route (i.e. rebuilding the markup) because it carries a lot of overhead in future updates to the image block client side.

Also, I worry we package a lot of this functionality in the image block alone, and any other block that uses images, misses on it.

This comment has been minimized.

Copy link
@azaozz

azaozz Nov 28, 2018

Author Contributor

Using block props to rebuild the <img> tags seems the best option. It would be trivial to add support for more/other attributes in the future.

Of course we can always go "the other route" like in the current processing when adding srcset and sizes: use regex to parse some of the attributes, then add or replace them with recalculated values.

@mtias

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

One general concern I have here is scalability if we want to reproduce a similar behaviour to WordPress/twentynineteen#629 for other blocks that are not wp:image, including third-party blocks that might be using media.

@azaozz

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

Heh, yoda conditionals fail for negative checks?? Who added that to the coding standards?? It was not a part of the original :)

Needs revision.

@azaozz

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

One general concern I have here is scalability if we want to reproduce a similar behaviour to WordPress/twentynineteen#629 for other blocks that are not wp:image, including third-party blocks that might be using media.

Right, the idea was to add that to the Image Block first, test it, tweak it, and then add support for all images in all blocks. It won't be necessary for all images to have all block props, would be enough to at least have width and height, or fileWidth and fileHeight. The "main" underlying idea is for all image tags to be filterable before output, preferably without using regex to parse the attributes.

Enabling support for other blocks would be as easy as changing (the negative yoda noncompliant) $block['blockName'] !== 'core/image' to include more blocks :)

@aduth

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

Heh, yoda conditionals fail for negative checks?? Who added that to the coding standards?? It was not a part of the original :)

Needs revision.

I'm not sure I follow. 'core/image' !== $block['blockName'] would be the corrected form, passing lint and complying with the standard?

@mtias mtias modified the milestones: 4.6, 4.7, 4.8 Nov 29, 2018

@youknowriad youknowriad removed this from the 4.8 milestone Dec 19, 2018

@getsource

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

@youknowriad I'm working on triage for WP 5.0.3, on https://core.trac.wordpress.org/ticket/45407, and noticed that this PR was moved out of all GB milestones.

Is still the approach planned to get the information to core? If it is, should it be milestoned somewhere so that it gets tracked? If not, I'd appreciate being linked to any other relevant issues or PRs so that I can add them to the core ticket.

Thanks so much!

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2019

Correct if I'm wrong but I think we kind of agreed that this PR is not the right approach to solve these issues. At least we should break it into more discrete items and discuss the path forward.

About the milestoning. PRs are milestoned according the corresponding plugin release. This didn't seem ready for me for 4.9 or 5.0. But the priorities... are handled more precisely in issues.

At the moment we have two milestones:

  • 5.0.x for the most pressing things
  • 5.x for the ones after

We also have "Phase 2" for things that can come more down the road. I'd like us to work and triage and clarify these milestones more. In my personal priorities for phase2, I consider this meta issue as a good one to follow #13113

@mor10

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2019

From my perspective, this PR is indeed too wide in scope and much of what it tries to solve deserves to be broken into smaller pieces or reallocated to a wider conversation about media in general.

Specifically regarding https://core.trac.wordpress.org/ticket/45407, this is still a pressing issue that needs a resolution immediately. Afaik it is possible to resolve that specific issue as proposed in the Trac ticket without altering large parts of core or GB. The main issue in the past seemed to be it requires both changes to Core and GB, but since GB is now core that shouldn't be a blocker.

I strongly recommend flagging the RICG ticket as high priority as core is still shipping incorrect code and preventing theme and plugin developers from resolving it on their end.

@gziolo

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

This didn't get to WordPress 5.0 and 5.1. What are the next steps planned, should it be refreshed? @youknowriad and @mapk any thoughts? It also sort of duplicates #11377 as it was extracted from it.

@aduth

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Per the most recent comments, it seems clear this pull request cannot be salvaged in its current state and will need to be revisited in separate smaller tasks.

Related tracking issues / tickets:

@aduth aduth closed this Apr 24, 2019

@aduth aduth deleted the fix/image-block-add-props branch Apr 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.