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.

Sign up for GitHub

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

Jump to bottom

Respect custom aspect ratio #52286

Merged
merged 3 commits into from
Jul 5, 2023
Merged

Respect custom aspect ratio #52286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
20b9223
add image width and height via css inline style, update width and hei…
draganescu Jul 4, 2023
fa4c8ac
keep width and height as attributes too, keep the attributes as numbers
draganescu Jul 4, 2023
3441ffb
updates image fixtures
draganescu Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 97 additions & 2 deletions packages/block-library/src/image/deprecated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { RichText, useBlockProps } from '@wordpress/block-editor';
import {
RichText,
useBlockProps,
__experimentalGetElementClassName as getBorderClassesAndStyles,
} from '@wordpress/block-editor';

/**
* Deprecation for adding the `wp-image-${id}` class to the image block for
Expand Down Expand Up @@ -539,4 +543,95 @@ const v5 = {
},
};

export default [ v5, v4, v3, v2, v1 ];
/**
* Deprecation for adding width and height as style rules on the inner img.
* It also updates the widht and height attributes to be strings instead of numbers.
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the width and height are still numbers here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the PR description remained behind on the actual end result of what's implemented. The updating to string never happened and this comment is wrong.

Copy link

Choose a reason for hiding this comment

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

The updating to string never happened and this comment is wrong.

Do you know if that is that stil true for WordPress 6.3.2? We're seeing a lot of 'block recovery' in the editor for images like these:

<!-- wp:image {"id":1,"width":200,"height":100,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large is-resized"><img src="https://example.com/wp-content/uploads/2023/10/example.svg" alt="" class="wp-image-1" style="width:200px;height:100px"/></figure>
<!-- /wp:image -->

Updating the width and height attributes from numbers to strings ("200px" and "100px") seems to fix the blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if that is that still true for WordPress 6.3.2?

Currently, they should be getting migrated to strings. However, it will only migrate for valid markup.

We're seeing a lot of 'block recovery' in the editor for images like these:

Yeah, that markup looks invalid. If the block attributes are correct, the img's width and height attributes should be there. Or if the markup is correct, the block attributes should be strings with px units.

Do you happen to know if that markup was generated with gutenberg trunk (at some point), a proper gutenberg plugin release, or a full WordPress release? If it was generated with some release code, we probably need to update some deprecations—we shouldn't be getting block recovery errors from release code.

Updating the width and height attributes from numbers to strings ("200px" and "100px") seems to fix the blocks.

Yep, this is what I was going to suggest. It's the best workaround for now.

Copy link

Choose a reason for hiding this comment

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

If it was generated with some release code, we probably need to update some deprecations—we shouldn't be getting block recovery errors from release code.

That is what I was thinking too.

Do you happen to know if that markup was generated with gutenberg trunk (at some point), a proper gutenberg plugin release, or a full WordPress release?

Not Gutenberg trunk. I assumed a full WordPress release, but I now discovered that the Gutenberg plugin is also installed (inactive, so I'm not sure if it has been active in the past and with which version).

We've updated all posts in the meantime, which has resolved the issue for our client.

Copy link
Contributor

Choose a reason for hiding this comment

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

#56916 was submitted, probably related to this PR.

Upgrading from 6.2 to 6.3 or 6.4 seems to break the Image block. This might be related to the fact that in 6.2, the width and height attribute values are number types.

*
* @see https://github.com/WordPress/gutenberg/pull/31366
*/
const v6 = {
save( { attributes } ) {
const {
url,
alt,
caption,
align,
href,
rel,
linkClass,
width,
height,
aspectRatio,
scale,
id,
linkTarget,
sizeSlug,
title,
} = attributes;

const newRel = ! rel ? undefined : rel;
const borderProps = getBorderClassesAndStyles( attributes );

const classes = classnames( {
[ `align${ align }` ]: align,
[ `size-${ sizeSlug }` ]: sizeSlug,
'is-resized': width || height,
'has-custom-border':
!! borderProps.className ||
( borderProps.style &&
Object.keys( borderProps.style ).length > 0 ),
} );

const imageClasses = classnames( borderProps.className, {
[ `wp-image-${ id }` ]: !! id,
} );

const image = (
<img
src={ url }
alt={ alt }
className={ imageClasses || undefined }
style={ {
...borderProps.style,
aspectRatio,
objectFit: scale,
} }
width={ width }
height={ height }
title={ title }
/>
);

const figure = (
<>
{ href ? (
<a
className={ linkClass }
href={ href }
target={ linkTarget }
rel={ newRel }
>
{ image }
</a>
) : (
image
) }
{ ! RichText.isEmpty( caption ) && (
<RichText.Content
className={ getBorderClassesAndStyles( 'caption' ) }
tagName="figcaption"
value={ caption }
/>
) }
</>
);

return (
<figure { ...useBlockProps.save( { className: classes } ) }>
{ figure }
</figure>
);
},
};

export default [ v6, v5, v4, v3, v2, v1 ];
2 changes: 2 additions & 0 deletions packages/block-library/src/image/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ export default function save( { attributes } ) {
...borderProps.style,
aspectRatio,
objectFit: scale,
width,
height,
Comment on lines +61 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

These need units since only numbers are saved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems it works without units by virtue of React being awesome. If the keys are width and height in a styles attribute in JSX the numbers will default to 'px`

} }
width={ width }
height={ height }
Expand Down
4 changes: 2 additions & 2 deletions test/integration/fixtures/blocks/core__image__deprecated-v1-add-responsive-class.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
"name": "core/image",
"isValid": true,
"attributes": {
"align": "left",
"url": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==",
"alt": "",
"caption": []
"caption": [],
"align": "left"
},
"innerBlocks": []
}
Expand Down
2 changes: 1 addition & 1 deletion test/integration/fixtures/blocks/core__image__deprecated-v2-add-is-resized-class.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
"name": "core/image",
"isValid": true,
"attributes": {
"align": "left",
"url": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==",
"alt": "",
"caption": [],
"align": "left",
"width": 100,
"height": 100
},
Expand Down
2 changes: 1 addition & 1 deletion ...tegration/fixtures/blocks/core__image__deprecated-v2-add-is-resized-class.serialized.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<!-- wp:image {"align":"left","width":100,"height":100} -->
<figure class="wp-block-image alignleft is-resized"><img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==" alt="" width="100" height="100"/></figure>
<figure class="wp-block-image alignleft is-resized"><img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==" alt="" style="width:100px;height:100px" width="100" height="100"/></figure>
<!-- /wp:image -->
2 changes: 1 addition & 1 deletion .../integration/fixtures/blocks/core__image__deprecated-v3-add-align-wrapper.serialized.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<!-- wp:image {"align":"left","width":100,"height":100} -->
<figure class="wp-block-image alignleft is-resized"><img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==" alt="" width="100" height="100"/></figure>
<figure class="wp-block-image alignleft is-resized"><img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==" alt="" style="width:100px;height:100px" width="100" height="100"/></figure>
<!-- /wp:image -->
2 changes: 1 addition & 1 deletion ...tegration/fixtures/blocks/core__image__deprecated-v4-remove-align-wrapper.serialized.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<!-- wp:image {"align":"left","id":13,"width":358,"height":164,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image alignleft size-large is-resized"><img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==" alt="" class="wp-image-13" width="358" height="164"/></figure>
<figure class="wp-block-image alignleft size-large is-resized"><img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==" alt="" class="wp-image-13" style="width:358px;height:164px" width="358" height="164"/></figure>
<!-- /wp:image -->