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

Adds Aspect ratio control on Image blocks in Grids #62891

Merged
merged 6 commits into from
Jun 28, 2024

Conversation

amitraj2203
Copy link
Contributor

What?

Fixes: #62889

Why?

Currently, when adding an Image block within a Grid layout, the Aspect Ratio control is disabled. This can limit user's ability to set consistent visual proportions across images within the grid. Enabling the Aspect Ratio control allows users to maintain uniformity in image display.

How?

Conditionally shows Aspect ratio control on grid layouts.

Testing Instructions

  1. Add a Grid block
  2. Add few images with different aspect ratios to the Grid
  3. Notice that the Aspect Ratio control is visible for the images

Screenshots or screencast

Screen.Recording.2024-06-27.at.2.56.58.AM.mov

@amitraj2203 amitraj2203 added [Type] Bug An existing feature does not function as intended [Block] Image Affects the Image Block [Block] Group Affects the Group Block (and row, stack and grid variants) labels Jun 26, 2024
Copy link

github-actions bot commented Jun 26, 2024

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: amitraj2203 <amitraj2203@git.wordpress.org>
Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>
Co-authored-by: ndiego <ndiego@git.wordpress.org>

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

@talldan talldan requested a review from tellthemachines June 27, 2024 05:39
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @amitraj2203!

I think ideally we'd find a solution that doesn't involve DimensionsTool having to know about layout. Perhaps instead of passing it parentLayoutType directly, we can introduce an array type prop with a list of tools to render. It could default to all tools, e.g. [ 'aspectRatio', 'widthHeight', 'scale'], but if we pass it ['aspectRatio'] it only renders the aspect ratio tool. Then we can leave deciding which tools to render based on parent layout type to the image block.

@amitraj2203
Copy link
Contributor Author

@tellthemachines Updated it as per your suggestion.

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this! The code is looking good now, there's only one small issue remaining. Currently, setting aspect ratio on images in a Grid block results in the images becoming distorted:
Screenshot 2024-06-28 at 10 57 40 AM

This is due to the object-fit property not being set. In Image blocks outside Grid, cover is being used as a default value for the scale attribute, which translates to the object-fit CSS property. Inside a grid, because we're not exposing the scale tool, I think the best solution is to set the attribute directly in aspectRatioControl, like so:

setAttributes( {
	aspectRatio: newAspectRatio,
	scale: 'cover',
} );

Apologies for not spotting this in my earlier review!

@tellthemachines
Copy link
Contributor

tellthemachines commented Jun 28, 2024

Oh, I just noticed something else in testing: the resizable box handles on the Image block are now appearing beneath the grid resizers:

Screenshot 2024-06-28 at 11 09 29 AM

We'll need to disable them, but I'm not 100% sure how. Looking into it now.

Update: changing the condition on this line to ! isResizable || parentLayoutType === 'grid' should stop the resizable box from rendering.

@amitraj2203
Copy link
Contributor Author

Thanks for reviewing it thoroughly. I have addressed the changes.

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for the update @amitraj2203! This is working well for me now.

@tellthemachines tellthemachines enabled auto-merge (squash) June 28, 2024 06:01
@tellthemachines tellthemachines merged commit 535c74f into WordPress:trunk Jun 28, 2024
61 checks passed
@github-actions github-actions bot added this to the Gutenberg 18.8 milestone Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block (and row, stack and grid variants) [Block] Image Affects the Image Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grid: Aspect ratio controls are disabled on Image blocks in Grids
2 participants