POC - Pass group block alignment context to image block #1704
Conversation
mukeshpanchal27
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
joemcgill
left a comment
joemcgill
left a comment
There was a problem hiding this comment.
Thanks @mukeshpanchal27. The updated tests are great for confirming whether this is working as intended. I've left a lot of feedback about how this could be simplified. Once you've addressed those, let's go ahead and merge this into the update/pass-alignment-by-context branch and continue iteration in that PR.
| if ( 'core/group' === $block['blockName'] || 'core/columns' === $block['blockName'] ) { | ||
| $context['is_parent_block'] = true; | ||
| $context['ancestor_block_align'] = $block['attrs']['align'] ?? ''; | ||
| } |
There was a problem hiding this comment.
Rather than passing both of these through as context, I think it would be better to just save the current alignment as context that can be observed by the child block during rendering.
In fact, I did a quick test of removing the is_parent_block context and all the tests still pass, so it doesn't seem necessary.
diff --git a/plugins/auto-sizes/includes/improve-calculate-sizes.php b/plugins/auto-sizes/includes/improve-calculate-sizes.php
index 072effdf..6a6f9e81 100644
--- a/plugins/auto-sizes/includes/improve-calculate-sizes.php
+++ b/plugins/auto-sizes/includes/improve-calculate-sizes.php
@@ -103,10 +103,9 @@ function auto_sizes_filter_image_tag( $content, array $parsed_block, WP_Block $b
$id = $block->attributes['id'] ?? 0;
$alignment = $block->attributes['align'] ?? '';
$width = $block->attributes['width'] ?? '';
- $is_parent_block = $block->context['is_parent_block'] ?? false;
$ancestor_block_align = $block->context['ancestor_block_align'] ?? '';
- return auto_sizes_calculate_better_sizes( (int) $id, (string) $size, (string) $alignment, (string) $width, (bool) $is_parent_block, (string) $ancestor_block_align );
+ return auto_sizes_calculate_better_sizes( (int) $id, (string) $size, (string) $alignment, (string) $width, (string) $ancestor_block_align );
};
// Hook this filter early, before default filters are run.
@@ -144,11 +143,10 @@ function auto_sizes_filter_image_tag( $content, array $parsed_block, WP_Block $b
* @param string $size The image size data.
* @param string $align The image alignment.
* @param string $resize_width Resize image width.
- * @param bool $is_parent_block Check if image block has parent block.
* @param string $ancestor_block_align The ancestor block alignment.
* @return string The sizes attribute value.
*/
-function auto_sizes_calculate_better_sizes( int $id, string $size, string $align, string $resize_width, bool $is_parent_block, string $ancestor_block_align ): string {
+function auto_sizes_calculate_better_sizes( int $id, string $size, string $align, string $resize_width, string $ancestor_block_align ): string {
$image = wp_get_attachment_image_src( $id, $size );
if ( false === $image ) {
@@ -158,19 +156,18 @@ function auto_sizes_calculate_better_sizes( int $id, string $size, string $align
// Retrieve width from the image tag itself.
$image_width = '' !== $resize_width ? (int) $resize_width : $image[1];
- if ( $is_parent_block ) {
- if ( 'full' === $ancestor_block_align && 'full' === $align ) {
- return auto_sizes_get_sizes_by_block_alignments( $align, $image_width, true );
- } elseif ( 'full' !== $ancestor_block_align && 'full' === $align ) {
- return auto_sizes_get_sizes_by_block_alignments( $ancestor_block_align, $image_width, true );
- } elseif ( 'full' !== $ancestor_block_align ) {
- $parent_block_alignment_width = auto_sizes_get_sizes_by_block_alignments( $ancestor_block_align, $image_width );
- $block_alignment_width = auto_sizes_get_sizes_by_block_alignments( $align, $image_width );
- if ( (int) $parent_block_alignment_width < (int) $block_alignment_width ) {
- return sprintf( '(max-width: %1$s) 100vw, %1$s', $parent_block_alignment_width );
- } else {
- return sprintf( '(max-width: %1$s) 100vw, %1$s', $block_alignment_width );
- }
+ // Handle different alignment use cases.
+ if ( 'full' === $ancestor_block_align && 'full' === $align ) {
+ return auto_sizes_get_sizes_by_block_alignments( $align, $image_width, true );
+ } elseif ( 'full' !== $ancestor_block_align && 'full' === $align ) {
+ return auto_sizes_get_sizes_by_block_alignments( $ancestor_block_align, $image_width, true );
+ } elseif ( 'full' !== $ancestor_block_align ) {
+ $parent_block_alignment_width = auto_sizes_get_sizes_by_block_alignments( $ancestor_block_align, $image_width );
+ $block_alignment_width = auto_sizes_get_sizes_by_block_alignments( $align, $image_width );
+ if ( (int) $parent_block_alignment_width < (int) $block_alignment_width ) {
+ return sprintf( '(max-width: %1$s) 100vw, %1$s', $parent_block_alignment_width );
+ } else {
+ return sprintf( '(max-width: %1$s) 100vw, %1$s', $block_alignment_width );
}
}
@@ -236,7 +233,7 @@ function auto_sizes_get_sizes_by_block_alignments( string $alignment, int $image
function auto_sizes_allowed_uses_context_for_image_blocks( array $uses_context, WP_Block_Type $block_type ): array {
if ( 'core/image' === $block_type->name ) {
// Use array_values to reset the array keys after merging.
- return array_values( array_unique( array_merge( $uses_context, array( 'is_parent_block', 'ancestor_block_align' ) ) ) );
+ return array_values( array_unique( array_merge( $uses_context, array( 'ancestor_block_align' ) ) ) );
}
return $uses_context;
}
@@ -253,7 +250,6 @@ add_filter( 'get_block_type_uses_context', 'auto_sizes_allowed_uses_context_for_
*/
function auto_sizes_modify_render_block_context( array $context, array $block ): array {
if ( 'core/group' === $block['blockName'] || 'core/columns' === $block['blockName'] ) {
- $context['is_parent_block'] = true;
$context['ancestor_block_align'] = $block['attrs']['align'] ?? '';
}
return $context;There was a problem hiding this comment.
If i didn't check the is_parent_block then it will break the existing test for Image block only.
It because it consider $ancestor_block_align alignment as None and return image size so instead on 100vw for image it return sizes="(max-width: 300px) 100vw, 300px" for Medium size image.
| * @param bool $print_sizes Print the sizes attribute. Default is false. | ||
| * @return string The sizes attribute value. | ||
| */ | ||
| function auto_sizes_get_sizes_by_block_alignments( string $alignment, int $image_width, bool $print_sizes = false ): string { |
There was a problem hiding this comment.
I don't understand changing the return type based on the $print_sizes parameter. If needed, it would be better to break this into two functions if necessary: one that calculates the width value, and another that formats the string for the sizes attribute.
There was a problem hiding this comment.
Initially, I considered separating them, but since the two functions are similar, I combined them into a single function by adding an extra parameter.
There was a problem hiding this comment.
+1 to separating them. Using a boolean to change function behavior is a code smell.
Ideally a separate function should take the return value of this function and format it, like @joemcgill suggested.
| // Retrieve width from the image tag itself. | ||
| $image_width = '' !== $resize_width ? (int) $resize_width : $image[1]; | ||
|
|
||
| if ( $is_parent_block ) { |
There was a problem hiding this comment.
I think a simpler way to do this is to compare the $ancestor_block_align value to the block's align attribute value, and use whichever is smaller in a single call to auto_sizes_get_sizes_by_block_alignments().
There was a problem hiding this comment.
See #1704 (comment) it get issue for single image block.
| $id = $block->attributes['id'] ?? 0; | ||
| $alignment = $block->attributes['align'] ?? ''; | ||
| $width = $block->attributes['width'] ?? ''; | ||
| $has_parent_block = isset( $parsed_block['parentLayout'] ); |
There was a problem hiding this comment.
The parentLayout added in WP 6.6. See https://github.com/WordPress/WordPress/blob/e174cbef2e3b7f1901892cb501f683c6e122231b/wp-includes/block-supports/layout.php#L926-L943.
Once we bump the version in #1682 it will fix the issue.
There was a problem hiding this comment.
In 5a9ce37 i bump the workflow WP version to 6.6
mukeshpanchal27
requested review from
felixarntz and
thelovekesh
as code owners
|
I’ll go ahead and merge this PR so we can iterate on it in the main PR. |
mukeshpanchal27
merged commit 2085845
into
update/pass-alignment-by-context
Summary
Related #1511.
It account for group as parent block and pass the context to the image block and based on the alignment information it return the accurate sizes value.
Relevant technical choices