Supply the extracted dimensions to images determined to need them #1117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
westonruter
commented
May 4, 2018
| @@ -157,30 +157,29 @@ private function determine_dimensions( $need_dimensions ) { | |||
| if ( ! $node instanceof DOMElement ) { | |||
| continue; | |||
| } | |||
| $height = $dimensions && isset( $dimensions['height'] ) ? $dimensions['height'] : self::FALLBACK_HEIGHT; | |||
| $width = $dimensions && isset( $dimensions['width'] ) ? $dimensions['width'] : self::FALLBACK_WIDTH; | |||
There was a problem hiding this comment.
I think $this->args['content_max_width'] should be used here if available. It got removed in #979.
westonruter
commented
May 4, 2018
| $node->setAttribute( 'width', $width ); | ||
| $node->setAttribute( 'height', $height ); | ||
| $class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size' : 'amp-wp-unknown-size'; | ||
| $node->setAttribute( 'class', $class ); | ||
| $node->setAttribute( 'class', trim( $class . ' amp-wp-unknown-size' ) ); |
There was a problem hiding this comment.
Is amp-wp-unknown-size even true here? Shouldn't it only be true if the FALLBACK_HEIGHT or FALLBACK_WIDTH are used? Otherwise, if we have $dimensions then we do know the size.
|
I'm working on another commit for this. |
westonruter
changed the title
westonruter
changed the title
* Update image tests to test for normal success case of image dimensions being extracted. * Add test case for when fallback dimensions are provided. * When image has one dimension but lacks another, ensure missing dimensions gets same ratio as underlying image when supplied. * Only add amp-wp-unknown-* classes if the dimensions are actually unknown (and fallbacks are used). * Restore use of content_max_width instead of FALLBACK_WIDTH if defined.
westonruter
changed the title
|
Approved Hi @westonruter, |
kienstra
reviewed
May 5, 2018
| * Class AMP_Img_Sanitizer_Test | ||
| * | ||
| * @covers AMP_Style_Sanitizer | ||
| */ |
There was a problem hiding this comment.
Nice addition of DocBlocks 😄
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a regression introduced in #979 whereby the dimensions determined are no longer actually supplied to the images that need them. Instead, the fallback image sizes are always used.
This issue is most plainly visible in Gutenberg where image blocks do not get output with
widthorheight.Additionally, this PR ensures that when a
widthorheightare missing that the missing dimension gets its counterpart supplied based on the ratio of the underlying extracted image dimensions. So given an image that is 1944⨉1191 andpost_contentthat contains:✅ This gets converted to:
❌ As opposed to:
Additionally, the
amp-wp-unknown-*classes will only be provided if the dimensions are actually unknown (and fallbacks are used).This PR also restores use of
content_max_widthif defined instead ofFALLBACK_WIDTH.