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

Image Aspect Ratio: Rely only on width to define image sizes when aspect ratio is set #53225

Closed
wants to merge 1 commit into from
Closed

Image Aspect Ratio: Rely only on width to define image sizes when aspect ratio is set #53225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 18 additions & 10 deletions packages/block-library/src/image/image.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,21 @@ export default function Image( {
const borderProps = useBorderProps( attributes );
const isRounded = attributes.className?.includes( 'is-style-rounded' );

const imgStyle = {
aspectRatio: aspectRatio ? aspectRatio : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the ternary needed here? What falsy values do we need to convert to undefined?

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's probably not needed it was just for my own ease of reading.

objectFit: scale,
...borderProps.style,
};
if ( aspectRatio ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed for the case where there is no width/height defined.

imgStyle.height = 'auto';
imgStyle.width = '100%';
}
if ( width ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will override the setting above.

imgStyle.width = width;
}
if ( height ) {
imgStyle.height = height;
}
let img = (
// Disable reason: Image itself is not meant to be interactive, but
// should direct focus to block.
Expand All @@ -564,14 +579,7 @@ export default function Image( {
} }
ref={ imageRef }
className={ borderProps.className }
style={ {
width:
( width && height ) || aspectRatio ? '100%' : undefined,
height:
( width && height ) || aspectRatio ? '100%' : undefined,
objectFit: scale,
...borderProps.style,
} }
style={ imgStyle }
/>
{ temporaryURL && <Spinner /> }
</>
Expand Down Expand Up @@ -689,8 +697,8 @@ export default function Image( {
onResizeStop();
setAttributes( {
width: elt.offsetWidth,
height: elt.offsetHeight,
aspectRatio: undefined,
height: 'auto',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main change. We are now relying on the width and aspect ratio to infer the height.

Copy link
Contributor

@ajlende ajlende Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It seems strange to me that dragging the bottom handle would set the width of the image. That interaction feels to me like it should be setting the height, but maybe that's just me.

There was someone who requested modifier keys for proportional resizing in #51843, that sort of interaction is what makes the most sense to me.

aspectRatio: aspectRatio ? aspectRatio : undefined,
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'm not sure why this wasn't set before?

Copy link
Contributor

Choose a reason for hiding this comment

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

The drag handles set both the width and the height so the aspectRatio needed to be cleared since it isn't valid with both width and height.

} );
} }
resizeRatio={ align === 'center' ? 2 : 1 }
Expand Down
22 changes: 14 additions & 8 deletions packages/block-library/src/image/save.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we could avoid editing save.js so we don't have to add a block deprecation.

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 don't know if this will require a deprecation anyway...

Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,26 @@ export default function save( { attributes } ) {
[ `wp-image-${ id }` ]: !! id,
} );

const imgStyle = {
...borderProps.style,
aspectRatio,
objectFit: scale,
};
if ( width ) {
imgStyle.width = width;
}

if ( height && ! isNaN( height ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why might height be NaN? We should try to prevent that from happening rather than checking isNaN().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Height can be 'auto', so maybe we should just check for that.

imgStyle.height = height;
}
const image = (
<img
src={ url }
alt={ alt }
className={ imageClasses || undefined }
style={ {
...borderProps.style,
aspectRatio,
objectFit: scale,
width,
height,
} }
style={ imgStyle }
width={ width }
height={ height }
height={ isNaN( height ) ? undefined : height }
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to prevent layout shifts when height is 'auto', we need to ensure that this property contains a real value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could keep the height for the image attributes and just use auto in the CSS

title={ title }
/>
);
Expand Down
Loading