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

Replace ImageSizeControl component with DimensionsTool component #53831

Open
wants to merge 13 commits into
base: trunk
Choose a base branch
from

Conversation

n2erjo00
Copy link
Contributor

What?

This PR replaces to-be-deprecated __experimentalImageSizeControl component with DimensionsTool component inside the media-text and latest-posts block.

Why?

Issue was introduced by ticket #52223 . __experimentalImageSizeControl component is marked to be deprecated in the some future, makes sense to get rid of it.

How?

Basically I opened up image block and started copy-pasting code from there.

Biggest curve ball was the requirement to use new components <ToolsPanel> and <ToolsPanelItem>

Testing Instructions

  1. Open a post or page
  2. Insert a media-text block and latest-posts block
  3. Observe the right side column for new settings and play around with them

Testing Instructions for Keyboard

Screenshots or screencast

Screenshot 2023-08-20 at 18 39 22
Screenshot 2023-08-20 at 18 39 53
Screenshot 2023-08-20 at 18 40 16

First screenshot is taken while latest-post block is active. Second screenshot is taken while latest-posts block is active and after clicking the three dots next to "Featured image" label. Last one is same as second screenshot but taken while media-text is active and instead label being "Featured image" it now was "Setting".

@n2erjo00
Copy link
Contributor Author

@aristath
Copy link
Member

aristath commented Nov 8, 2023

@n2erjo00 Could you please rebase this PR to resolve conflicts?
Now that WP 6.4 is released, contributors are not so stressed and will have more time to review this 👍

@n2erjo00
Copy link
Contributor Author

n2erjo00 commented Nov 8, 2023

@aristath Trunk merged

@carolinan carolinan requested a review from t-hamano November 17, 2023 08:52
@carolinan carolinan added [Type] Enhancement A suggestion for improvement. [Block] Latest Posts Affects the Latest Posts Block [Block] Media & Text Affects the Media & Text Block labels Nov 17, 2023
"featuredImageScale": {
"type": "string"
},
"featuredImageAspectRation": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a typo? Did you mean featuredImageAspectRatio?

@carolinan
Copy link
Contributor

carolinan commented Nov 17, 2023

I am not sure this control is going to work for the media & text block, because it has a different type of media width setting which is meant to work for both images and video.
When I use the drag handle, the value in the image width control does not change. Compare to the image block, where the resize does update the value in the control.
When I enable the option "Crop image to fill entire column", there is a JavaScript warning in the browser console.

The settings work better for the latest posts block.
The alignments for the image are not so visible when the user chooses a large size, but it is probably a none issue.

@t-hamano
Copy link
Contributor

Thanks for the PR, @n2erjo00!

This PR not only replaces deprecated components, but also adds new attributes such as width, height, ratio, and scale.

Merging this PR would require a broader discussion, especially regarding newly added features. We're also updating the save function for the Media and Text block, so we'll need the block deprecation code as well. E2E testing for new attributes may also be required.

Personally, I think we can focus on replacing deprecated components first and consider adding new features later.

For example, taking the Media and Text block as an example, this means dividing them into the smallest PRs as follows:

  1. Refactor the setting panel using ToolsPanel and ToolsPanelItem component (this is because ResolutionTool consists of the ToolsPanelItem component)
  2. Replace deprecated components with the ResolutionTool component
  3. Explore the possibility of adding width/height/scale/ratio attributes via the DiemensionTools component

Even if this PR is closed, the code will still be available for reference by those working on these tasks.

ccing @ajlende - Because I think you are very familiar with these components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Latest Posts Affects the Latest Posts Block [Block] Media & Text Affects the Media & Text Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants