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

Update/image block #11377

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
9d5091f
Update image block
azaozz Jun 11, 2018
4b3d889
Update image block, add srcset to attachment data
azaozz Jun 11, 2018
be16a73
Image block: fix docs
azaozz Jun 11, 2018
041d1a5
Merge master
azaozz Aug 28, 2018
64648c1
Fixes after merging from master. (NOTE: JSHint still throws errors an…
azaozz Aug 28, 2018
28d9315
Merge branch 'master' into origin/update/image-block
azaozz Sep 7, 2018
0a73bc9
Merge branch 'master' into origin/update/image-block
azaozz Sep 10, 2018
1d0d17e
Fixes after update
azaozz Sep 10, 2018
dbba831
Use scale attribute instead of width and heightwq
youknowriad Sep 30, 2018
b6dc78f
Merge branch 'master' into update/image-block
azaozz Oct 8, 2018
108ea54
Merge branch 'master' into update/image-block
azaozz Oct 9, 2018
22207ba
Interim - img block
azaozz Oct 15, 2018
4cb8189
Merge branch 'master' into update/image-block
azaozz Oct 15, 2018
ae95eab
In progress
azaozz Oct 15, 2018
d9ef233
Merge branch 'master' into update/image-block
azaozz Oct 17, 2018
0aea270
Almost complete!
azaozz Oct 18, 2018
a476203
In progress
azaozz Oct 19, 2018
8d37181
Merge branch 'master' into update/image-block
azaozz Oct 19, 2018
9d0899e
Merge branch 'master' into update/image-block
azaozz Oct 19, 2018
22007e6
In progress
azaozz Oct 20, 2018
5b5be6f
Merge branch 'master' into update/image-block
azaozz Oct 20, 2018
5525986
Complete?
azaozz Nov 1, 2018
83ca683
Merge branch 'master' into update/image-block
azaozz Nov 1, 2018
15520ea
Merge branch 'master' into update/image-block
azaozz Nov 1, 2018
17c5563
A bit of cleanup
azaozz Nov 1, 2018
6a98975
Merge branch 'master' into update/image-block
azaozz Nov 1, 2018
e8dfda2
Fix missing comment
azaozz Nov 2, 2018
e1a5a86
Merge branch 'master' into update/image-block
azaozz Nov 6, 2018
39bbfec
Merge branch 'master' into update/image-block
azaozz Nov 6, 2018
5102f35
Re-enable linebreak style after linting on Windows...
azaozz Nov 6, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .eslintrc.js
Expand Up @@ -181,6 +181,8 @@ module.exports = {
};
} ),
} ],
// TODO: removeme, maybe? Without this linting fails horribly on Windows, many thousands of false positives :)
// 'linebreak-style': 'off',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. Still work-in-progress though, will do the TODO :)

},
overrides: [
{
Expand Down
Empty file modified bin/bootstrap-env.sh 100755 → 100644
Empty file.
Empty file modified bin/install-wordpress.sh 100755 → 100644
Empty file.
Empty file modified bin/reset-e2e-tests.sh 100755 → 100644
Empty file.
35 changes: 14 additions & 21 deletions lib/blocks.php
Expand Up @@ -120,27 +120,20 @@ function get_dynamic_block_names() {
*/
function get_dynamic_blocks_regex() {
$dynamic_block_names = get_dynamic_block_names();
$dynamic_block_pattern = (
'/<!--\s+wp:(' .
str_replace(
'/',
'\/', // Escape namespace, not handled by preg_quote.
str_replace(
'core/',
'(?:core/)?', // Allow implicit core namespace, but don't capture.
implode(
'|', // Join block names into capture group alternation.
array_map(
'preg_quote', // Escape block name for regular expression.
$dynamic_block_names
)
)
)
) .
')(\s+(\{.*?\}))?\s+(\/)?-->/'
);

return $dynamic_block_pattern;

// Escape block name for regular expression.
$dynamic_block_names = array_map( 'preg_quote', $dynamic_block_names );

// Join block names into capture group alternation.
$dynamic_block_names = implode( '|', $dynamic_block_names );

// Allow implicit core namespace, but don't capture.
$dynamic_block_names = str_replace( 'core/', '(?:core/)?', $dynamic_block_names );

// Escape namespace, not handled by preg_quote.
$dynamic_block_names = str_replace( '/', '\/', $dynamic_block_names );

return '/<!--\s+wp:(' . $dynamic_block_names . ')(\s+(\{.*?\}))?\s+(\/)?-->/';
}
}

Expand Down
60 changes: 60 additions & 0 deletions lib/compat.php
Expand Up @@ -116,6 +116,66 @@ function gutenberg_wpautop( $content ) {
remove_filter( 'the_content', 'wpautop' );
add_filter( 'the_content', 'gutenberg_wpautop', 6 );

/**
* Add `srcset` to the image meta `sizes` array in the REST API response.
*
* @param WP_REST_Response $response Response object.
* @param WP_Post $attachment Attachment post object.
* @return WP_REST_Response Response object.
*/
function gutenberg_rest_prepare_attachment( $response, $attachment ) {
if ( ! isset( $response->data['media_type'] ) ) {
return $response;
}

if ( $response->data['media_type'] === 'image' && ! empty( $response->data['media_details']['sizes'] ) ) {
$meta = wp_get_attachment_metadata( $attachment->ID );
$sizes = $response->data['media_details']['sizes'];

foreach ( $sizes as $size_name => $size ) {
$srcset = wp_calculate_image_srcset( array( $size['width'], $size['height'] ), $size['source_url'], $meta, $attachment->ID );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting solution, but I wonder if it would be better to create the equivalent of wp_calculate_image_srcset() as a JS function that could be used on the front end when necessary. I'm not sure what the performance impact is of calculating the srcset of every possible image size included in an image's sizes attribute, but I imagine it isn't insignificant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, for the REST API, we should open a ticket to add this properly via register_rest_field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looked into making a JS version of wp_calculate_image_srcset() however it will lack all the filtering we currently do in PHP. Also it gets pretty complex when dealing with custom image sizes added by themes and plugins (we can't even "see" any custom sizes in JS, they are in a PHP global).

There is a performance hit for recalculating the srcset for each image size but it is pretty small as we already have the attachment meta cached at that time (that's by far the slowest part when calculating srcset). Also, this is done only when editing a post and opening the media modal, i.e. there's no slow down from the user's perspective.

A good optimization would be to calculate only one srcset per image, for the "large" size, (or the "full" size if the image is small and "large" doesn't exist). However that leaves out the cases where custom sizes were added by a theme.

Also, in order to handle this better in JS we may need to pass the srcset as an array/object (not as string) where the keys are the "size names" so we can look into it easier.

Additionally, for the REST API, we should open a ticket to add this

Right. Both of these functions will be "merged" to the appropriate places once this is added to core.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for the explanation. I don't think it would be out of the question to add a JS hook to a javascript function, but if the performance hit is negligible, I'm not opposed to including that data in REST responses.

Perhaps doing one srcset per image is something we could improve in a future release in combination with something like "image sets" where we group sets of images from the same crop into an array, where we could have a single srcset per array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...where we group sets of images from the same crop into an array, where we could have a single srcset per array

Right, good idea! We also need to look a bit more into the "image meta", can expand it a bit and store some more info about the sub-sizes that were created, when, why, etc. Lets schedule a review of that in 5.1?


if ( $srcset ) {
$sizes[ $size_name ]['srcset'] = $srcset;
}
}

$response->data['media_details']['sizes'] = $sizes;
}

return $response;
}
add_filter( 'rest_prepare_attachment', 'gutenberg_rest_prepare_attachment', 10, 2 );

/**
* Add `srcset` to the prepared attachment data for the Media Library.
*
* @param array $response Array of prepared attachment data.
* @param WP_Post $attachment Attachment object.
* @param array $meta Array of attachment meta data.
* @return array Array of prepared attachment data.
*/
function gutenberg_prepare_attachment_for_js( $response, $attachment, $meta ) {
if ( ! empty( $response['type'] ) && $response['type'] === 'image' && ! empty( $response['sizes'] ) ) {
foreach ( $response['sizes'] as $size_name => $size ) {
$srcset = wp_calculate_image_srcset( array( $size['width'], $size['height'] ), $size['url'], $meta, $attachment->ID );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, lets handle this the same as above.


if ( $srcset ) {
$response['sizes'][ $size_name ]['srcset'] = $srcset;
}

if ( array_key_exists( $size_name, $meta['sizes'] ) ) {
$response['sizes'][ $size_name ]['actual_size'] = array(
'width' => (int) $meta['sizes'][ $size_name ]['width'],
'height' => (int) $meta['sizes'][ $size_name ]['height'],
);
}
}
}

return $response;
}
add_filter( 'wp_prepare_attachment_for_js', 'gutenberg_prepare_attachment_for_js', 10, 3 );

/**
* Check if we need to load the block warning in the Classic Editor.
Expand Down
4 changes: 3 additions & 1 deletion lib/load.php
Expand Up @@ -51,7 +51,6 @@
require dirname( __FILE__ ) . '/i18n.php';
require dirname( __FILE__ ) . '/register.php';


// Register server-side code for individual blocks.
if ( ! function_exists( 'render_block_core_archives' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/archives/index.php';
Expand All @@ -74,3 +73,6 @@
if ( ! function_exists( 'render_block_core_shortcode' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/shortcode/index.php';
}
if ( ! function_exists( 'render_block_core_image' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/image/index.php';
}