Skip to content

Fix: Refactor Featured Image Block to use aspectRatio block support - #61485

Open
hbhalodia wants to merge 2 commits into
WordPress:trunkfrom
hbhalodia:fix/issue-61432
Open

Fix: Refactor Featured Image Block to use aspectRatio block support#61485
hbhalodia wants to merge 2 commits into
WordPress:trunkfrom
hbhalodia:fix/issue-61432

Conversation

@hbhalodia

@hbhalodia hbhalodia commented May 8, 2024 •
edited by Mamaduka
Loading

Copy link
Copy Markdown
Contributor

What?

Resolves #61432

Why?

  • To improve the code quality, we have removed the aspectRatio from attributes and used the Block Supports for the core/post-featured-image, because the attributes were redundant.

How?

  • Have removed the aspectRatio from the attributes.
  • Have added the block supports, dimensions[aspectRatio] in block.json.
  • Used that value to update the block in editor and frontend as well.

Testing Instructions

  1. Open a post/page.
  2. Added the core/post-featured-image block.
  3. Check the aspect ratio dropdown. It is now using the block supports, instead of native attributes.

Testing Instructions for Keyboard

  • NIL

Screenshots or screencast

  • NIL, code quality improvement.

@github-actions

github-actions Bot commented May 8, 2024 •
edited
Loading

Copy link
Copy Markdown

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.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @eric-michel.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: eric-michel.

Co-authored-by: hbhalodia <hbhalodia@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>

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

@hbhalodia hbhalodia mentioned this pull request May 8, 2024
Open
@Mamaduka Mamaduka added [Type] Enhancement A suggestion for improvement. [Block] Post Featured Image Affects the Post Featured Image Block labels May 8, 2024
@Mamaduka
Mamaduka requested review from Mamaduka, abotteram and ajlende and removed request for abotteram, ajitbohra, juanmaguitar, ndiego and ryanwelcher May 8, 2024 12:02
@Mamaduka

Mamaduka commented May 8, 2024

Copy link
Copy Markdown
Member

I've not properly tested this, but I think the Featured Image won't retain the aspect ratio after the update.

  1. Set the aspect ratio for a featured image on the trunk. Save the post.
  2. Switch to this branch.
  3. See if the aspect ratio is still set.

@hbhalodia

hbhalodia commented May 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I've not properly tested this, but I think the Featured Image won't retain the aspect ratio after the update.

  1. Set the aspect ratio for a featured image on the trunk. Save the post.
  2. Switch to this branch.
  3. See if the aspect ratio is still set.

Hi @Mamaduka, I tried the above steps and yes, the aspectRatio which is set is no more used. We need to make sure it is backward compatible, if aspectRatio is set, it should respect that, and if changed then we can use new one.

Let me know if you have some idea how can we proceed with that? I tried some findings but did not succeed. Maybe we should retain the attribute and check if that is set, use that, and once change it from block support make it undefined. I am not sure what could be the better approach.

Thank You.

hbhalodia
hbhalodia commented May 9, 2024
View reviewed changes
Comment thread packages/block-library/src/post-featured-image/dimension-controls.js
} ) );

// Get the block Supports aspect ratio.
const aspectRatio = attributes?.style?.dimensions?.aspectRatio;

@hbhalodia hbhalodia May 9, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

const aspectRatio = attributes?.style?.dimensions?.aspectRatio;

Above line can be changed to something like,

const blockSupportAspectRatio = attributes?.style?.dimensions?.aspectRatio || aspectRatio;

Here we should not remove the attribute from the block itself, we just keep it to get the value which was stored previously?

But it would have issue like, blockSupport aspectRatio dropdown would not show the selected aspectRatio from the atttribute.

@eric-michel

eric-michel commented May 23, 2024

Copy link
Copy Markdown

@hbhalodia can you confirm if this implementation of aspect-ratio will create an inline style even when an aspect-ratio is not selected? This happened with the Cover block and broke a lot of our sites. If this were to happen with the Group block, it would be catastrophic for us.

@aaronrobertshaw aaronrobertshaw mentioned this pull request Sep 8, 2026
Open

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabiankaegy fabiankaegy Awaiting requested review from fabiankaegy fabiankaegy is a code owner
@Mamaduka Mamaduka Awaiting requested review from Mamaduka
@ajlende ajlende Awaiting requested review from ajlende

At least 1 approving review is required to merge this pull request.

Assignees

@hbhalodia hbhalodia

Labels

[Block] Post Featured Image Affects the Post Featured Image Block [Type] Enhancement A suggestion for improvement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor Featured Image Block to use aspectRatio block support

3 participants

@hbhalodia @Mamaduka @eric-michel