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

Accurate sizes: Use block context approach #1625

Open
wants to merge 10 commits into
base: feature/1511-incorporate-layout-constraints-from-ancestors
Choose a base branch
from

Conversation

mukeshpanchal27
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 commented Oct 23, 2024

Summary

These PR updates the existing code to use the new block context approach that we discussed in #1511 (comment)

@mukeshpanchal27 mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature no milestone PRs that do not have a defined milestone for release [Plugin] Enhanced Responsive Images Issues for the Enhanced Responsive Images plugin (formerly Auto Sizes) labels Oct 23, 2024
@mukeshpanchal27 mukeshpanchal27 self-assigned this Oct 23, 2024
@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review October 24, 2024 11:02
Copy link

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: westonruter <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

* @param array{ attrs?: array{ align?: string, width?: string } } $parsed_block The parsed block.
* @return string The updated block content.
* @param string|mixed $content The HTML content.
* @param string $context Optional. Additional context to pass to the filters.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param string $context Optional. Additional context to pass to the filters.
* @param string|null $context Optional. Additional context to pass to the filters.

Copy link
Member

Choose a reason for hiding this comment

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

Or per below:

Suggested change
* @param string $context Optional. Additional context to pass to the filters.

*/
function auto_sizes_filter_image_tag( string $content, array $parsed_block ): string {
function auto_sizes_prime_attachment_caches( $content, string $context = null ): string {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function auto_sizes_prime_attachment_caches( $content, string $context = null ): string {
function auto_sizes_prime_attachment_caches( $content, ?string $context = null ): string {

Copy link
Member

Choose a reason for hiding this comment

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

Or per below:

Suggested change
function auto_sizes_prime_attachment_caches( $content, string $context = null ): string {
function auto_sizes_prime_attachment_caches( $content ): string {

Comment on lines +155 to +157
if ( null === $context ) {
$context = current_filter();
}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps my above comments should be adjusted to delete the $context param altogether because actually the variable isn't being used at all. So should it just be removed?

Suggested change
if ( null === $context ) {
$context = current_filter();
}
if ( null === $context ) {
$context = current_filter();
}

Copy link
Member

Choose a reason for hiding this comment

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

Also, as it stands right now, the 2nd parameter is never passed in anyway:

add_filter( 'the_content', 'auto_sizes_prime_attachment_caches', 6 );

// Resize image width.
if ( isset( $parsed_block['attrs']['width'] ) ) {
$processor->set_attribute( 'data-resize-width', $parsed_block['attrs']['width'] );
if ( preg_match( '/wp-image-([0-9]+)/i', $class, $class_id ) === 1 ) {
Copy link
Member

Choose a reason for hiding this comment

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

This should ensure that integers starting at 1 are only considered and that the class wp-image-123 class is only considered, not my-wp-image-123. Granted, this is very unlikely. Also, there doesn't seem to be a need to do a case-insensitive check.

Suggested change
if ( preg_match( '/wp-image-([0-9]+)/i', $class, $class_id ) === 1 ) {
if ( preg_match( '/(?:^|\s)wp-image-([1-9][0-9]*)(?:\s|$)/', $class, $class_id ) === 1 ) {

if ( isset( $parsed_block['attrs']['width'] ) ) {
$processor->set_attribute( 'data-resize-width', $parsed_block['attrs']['width'] );
if ( preg_match( '/wp-image-([0-9]+)/i', $class, $class_id ) === 1 ) {
$attachment_id = absint( $class_id[1] );
Copy link
Member

Choose a reason for hiding this comment

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

Since a negative will never be encountered anyway, seems we can just cast this here:

Suggested change
$attachment_id = absint( $class_id[1] );
$attachment_id = (int) $class_id[1];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no milestone PRs that do not have a defined milestone for release [Plugin] Enhanced Responsive Images Issues for the Enhanced Responsive Images plugin (formerly Auto Sizes) [Type] Enhancement A suggestion for improvement of an existing feature
Projects
Status: Code Review 👀
Development

Successfully merging this pull request may close these issues.

2 participants