Add image block aspect ratio control #51545
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ajlende
force-pushed
the
add/image-block-aspect-ratio-control
branch
2 times, most recently
from
4a060cd to
cd5e640
Compare
|
Size Change: +9.91 kB (+1%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 652301c3eadadf2f84b36149e941acea3ab248c5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5350708953
|
ajlende
added
[Block] Image
ajlende
force-pushed
the
add/image-block-aspect-ratio-control
branch
4 times, most recently
from
3e3ae29 to
bf82b52
Compare
|
There are still a few TODOs in the comments and tests need to be updated, but I wouldn't mind some early reviews, so I've opened this up as ready for review. |
ajlende
requested review from
draganescu,
scruffian,
getdave,
jeryj,
richtabor,
MaggieCabrera and
jasmussen
|
This is really great, thanks for the PR! Let's see if we can't get this in 6.3 it's so close. Here's a GIF showing the aspect ratio dropdown, how it affects scaling after the fact, the fill control, and even a custom aspect ratio when both width and height are entered:
There is one thing that I would like us to fix, but which shouldn't be blocking for the PR. Here's a testimonial pattern with a circular avatar that I insert. Note two things:
So it seems like there's an issue with that separate pattern. But nevertheless even when recovered, there's an issue we should fix, which is that when a custom aspect ratio is applied, dimensions and aspect ratio should be retained across image replacements. In the following GIF I drop a big landscape image onto the small circular avatar:
Notice how the small circular avatar becomes a bit pill shape instead. Ideally, and this can be separate, the dimensions should've been retained when the custom aspect ratio is selected. A few small style tweak. When there's no segmented control for scale mode, there's too much space below the aspect ratio dropdown: 
I personally think that the "fill" mod of this segmented control is completely useless and isn't something we should surface to users: 
I realize it's already shipping and for featured image too. But could we / should we remove that option? |
getdave
reviewed
Jun 20, 2023
getdave
reviewed
Jun 20, 2023
getdave
reviewed
Jun 20, 2023
getdave
reviewed
Jun 20, 2023
packages/block-editor/src/components/dimensions-tool/scale-tool.js
Outdated
Show resolved
Hide resolved
getdave
reviewed
Jun 20, 2023
|
Just to clarify, the intent here was to preserve the dimensions of the image even when dropping in a replacement, so the metrics of a pattern are retained even if you update the imagery inside. That's addressed here, and working really well 👌 |
|
Thanks for your clarification:
|
|
👌 |
bph
added
Needs Dev Note
69 tasks
2 tasks
anomiex
reviewed
Jul 10, 2023
| @@ -32,6 +32,7 @@ | |||
| ], | |||
| "dependencies": { | |||
| "@babel/runtime": "^7.16.0", | |||
| "@emotion/styled": "^11.6.0", | |||
There was a problem hiding this comment.
This results in an unexpected transitive peer dependency on @emotion/react. See #52474 for details.
anomiex
added a commit
to anomiex/gutenberg
that referenced
this pull request
Jul 10, 2023
PR WordPress#51545 added a dependency on `@emotion/styled`, which resulted in a transitive peer dependency on `@emotion/react`. We should satisfy that dependency ourself, just as we do in the components package.
Mamaduka
pushed a commit
that referenced
this pull request
Jul 11, 2023
PR #51545 added a dependency on `@emotion/styled`, which resulted in a transitive peer dependency on `@emotion/react`. We should satisfy that dependency ourself, just as we do in the components package.
33 tasks
ramonjd
reviewed
Jul 12, 2023
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
* Simplify ImageSizeControl by using Auto as a placeholder * Rename imageWidth and imageHeight props to naturalWidth and naturalHeight * Convert NumberControl onChange values to Numbers * Simplify LatestPostsEdit to use updated ImageSizeControl * Add JSDoc types for debugging * Remove unnecessary noop * Fix possible undefined values in NumberControl onChange * Fix onChangeImage param type which may be undefined * Rename OnChange callback prop * Inline JSDoc props instead of new object * Simplify handing undefined and NaN in onChange * Revert prop name change since this isn't a private API * Add a privateApis export for experimental ImageSizeControl * Use the privateApis version of ImageSizeControl * Add deprecation notice to the original component * Revert image-size-control and create image-dimensions-control instead * Re-add deprecation notice to image-size-control * Try making a whole new component * Revert changes to image, latest-posts, and media-text blocks * Organize and update the dimensions tool panel item * Reword size help text * Reorganize into reusable components * Add stories for other individual tools * Update stories path * Remove SelectControl __next prop * Pass through isShownByDefault to ResolutionTool * Remove unused scss * Deprecate experimental ImageSizeControl * Simplify ScaleTool onChange * Add better defaults for value and onChange * Fix circular dependency * Update comment about auto and custom aspect ratios * Add JSDoc types for ScaleTool * Add JSDoc types for WidthHeightTool * Add default value and onChange for WidthHeightTool * Remove unused import * Add aspectRatio to image block attributes * Add scale to image block attributes * Update JSDoc comment * Add dimensions tool to image block * Rename naturalAspectRatio for clarity * Fix aspect-ratio-tool lint * Fix scale-tool lint * Fix width-height-tool lint * Fix dimensions-tool lint * Fix resolution-tool lint * Add @emption/styled to block-editor * Fix image block lint * Update components changelog * Fix AspectRatioTool reference * Support 'auto' in width-height-tool * Make null/undefined values mean 'auto' instead of defaultValue in aspectRatioTool * Add deprecation for image block * Fix ResizableBox interactions * Add comments for default values * Fix ResizableBox with auto w/h * Clear aspect-ratio on resize * Add TODO comment for ResolutionTool defaultValue * Move the scale hide/show into dimensions controls * Add first test * Fix scale being set after it was deleted * WIP writing tests * Update test * UI tweaks * Move alt text as ToolsPanelItem * Tweak default scale option help text * Only use contain and cover for image scale options * Update test * Test the remaining callback values * Add comment about toStrictEqual * Add test for setting custom aspect ratio and then resetting * Move custom scaleOptions to the image block * Remember last aspect ratio so it can be restored when with/height are unset then set * Remove unused import * Format code * Remove image w/h reset when a new image is added * Use UnitControl's default units instead of spacing.units * Provide the complete set of object-fit options by default * Update TODO that will be committed * Clean up evalAspectRatio and add docs * Someone can file a bug report if offsetWidth/offsetHeight causes issues * I couldn't figure out why height depended on having a custom border, but things seem to work without that * Update docs for image block * Update comment about default value * Fix redundant wording * I think the img width and height attributes can be removed if they're specified in the style attribute * Update package-lock.json with @emotion/styled dependency * Update mock calls for test example * Simplify test values * Consolidate mock calls expect * Require defaultScale and defaultAspectRatio for DimensionsTool * Add DimensionsTool tests for all custom transitions * Remove comment about matching aspect ratio options * Remove redundant check in tests * Add comments to defaultAspectRatio and defaultScale * Organize tests by which field is being updated * Fix type conversion * Add state diagram for last two tests * Refactor and fix some tests * Fix and simplify WidthHeightTool onChange * Remove default scale option in image block.json * Simplify DimensionsTool onChange logic * Update block deprecations with width and height * Revert image block width and height attributes to numbers since we only support px units for now * Revert "Update block deprecations with width and height" This reverts commit 941a81149ed4bc344ac2c0e183624069e33d75ad. * Prevent NaN width/height * Fix DimensionTool width/height units * Fix JSDoc Dimenstions width/height types * No default needed for ResolutionTool * Fix drag handle aspect ratio reset * Simplify null checks * Stop using pxWidth and pxHeight * Remove e2e tests that reference the scale button that was removed * Fix image scaling for small images * Try fixing aspectRatio only images * Update test to respect the new aspect ratio behavior --------- Co-authored-by: Alex Lende <alex@lende.xyz> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
Mamaduka
reviewed
Oct 3, 2023
Comment on lines
+36
to
+37
| alternative: | ||
| 'wp.blockEditor.privateApis.DimensionsTool and wp.blockEditor.privateApis.ResolutionTool', |
There was a problem hiding this comment.
I just noticed this message when testing the "Media & Text" block, which still uses the deprecated component.
This alternative suggestion isn't helpful for third-party consumers since the private APIs aren't available for them. Should we remove this from the deprecation message?
There was a problem hiding this comment.
Also just noting here since I just ran into this yesterday, it isn't fun for third party extenders when a component is deprecated whilst the replacement is still marked as private 😞
|
I just noticed a bug with resetting the alt text field, that I think might have been introduced in this PR. I have a potential fix up over in #56809. |
|
@andrewserong only since I'm capturing you here, if you get a moment of time, can you look at whether #54032 is something that could be fixable? I'd love to pair up with you (or anyone who has time) to work on that one. It's no more urgent than everything else, so this is only an appeal in case it's workable. |
|
Thanks for the ping, Joen! That does sound like a very cool feature, I'll add a couple of comments over on #54032. |
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Adds a new dimensions controller that includes aspect-ratio, object-fit (size), width, and height.
Why?
Fixes #51138
How?
Testing Instructions
Storybook
npm run storybook:dev).Image Block
Testing Instructions for Keyboard
Same as above, with the keyboard.
Screenshots or screencast