Accurate sizes: Calculate image sizes during block rendering #1625
Conversation
mukeshpanchal27
added
[Type] Enhancement
…ors' into update/use-filter-approach
|
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. |
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
|
We are tracking the unit test issue here #1634 |
There was a problem hiding this comment.
LGTM, although @joemcgill would know better what to look for in what this is seeking to achieve.
|
Once #1663 get merged the unit tests issue will be resolved. |
…ors' into update/use-filter-approach
There was a problem hiding this comment.
After chatting with @mukeshpanchal27 about this PR, I now better understand that the purpose of this change is to replace the current implementation for more accurate sizes from the initial approach, which filters core's automatic sizes calculation in wp_calculate_image_sizes, to a new process that adds sizes attributes to blocks during the block rendering process, so that we can make use of block information that is available during the block rendering process that is not currently available to the wp_calculate_image_sizes filter as it is currently implemented in Core.
A small nitpick for a future changelog — the title of this PR would more accurately be: Accurate sizes: calculate image sizes during block rendering
I've left some additional feedback inline. Once this is updated, I think we can merge this and use this as a starting point for incorporating other block information (e.g., block context) into the calculation logic.
| if ( isset( $parsed_block['attrs']['align'] ) ) { | ||
| $processor->set_attribute( 'data-align', $parsed_block['attrs']['align'] ); | ||
| $images = array(); | ||
| while ( $processor->next_tag( array( 'tag_name' => 'IMG' ) ) ) { |
There was a problem hiding this comment.
This seems like a good use of the WP_HTML_Tag_Processor, and is fine for a first pass at this functionality, but it would be good to do some profiling of this method against the preg_match_all approach that is currently in use in Core.
If we eventually want to land this code in WP, we'll want to move this priming logic from wp_filter_content_tags rather than duplicate it in two places, at which time we should be mindful of any performance differences between the current logic and this slightly updated version that makes use of the WP_HTML_Tag_Processor.
There was a problem hiding this comment.
This seems like a good use of the
WP_HTML_Tag_Processor, and is fine for a first pass at this functionality, but it would be good to do some profiling of this method against thepreg_match_allapproach that is currently in use in Core.
Haven't we already debated this at length, with the conclusion that using WP_HTML_Tag_Processor is preferred for correctness and that the practical performance impact is inconsequential?
There was a problem hiding this comment.
In previous discussion in #1471 we preferred to use WP_HTML_Tag_Processor.
There was a problem hiding this comment.
Haven't we already debated this at length, with the conclusion that using WP_HTML_Tag_Processor is preferred for correctness and that the practical performance impact is inconsequential?
I keep seeing this same question asked in almost every PR where WP_HTML_Tag_Processor is used in place of other approaches, so if a consensus conclusion was reached, I was not aware of it. I'm more than happy for this to be the decision though.
mukeshpanchal27
changed the title
joemcgill
merged commit 6e9f892
into
feature/1511-incorporate-layout-constraints-from-ancestors
Summary
These PR updates the existing code to use the new block context approach that we discussed in #1511 (comment)