Add aspect ratio to image placeholder #54216
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
|
Size Change: +10.1 kB (+1%) Total Size: 1.53 MB
ℹ️ View Unchanged
|
draganescu
added
[Type] Enhancement
scruffian
reviewed
Sep 6, 2023
| @@ -726,6 +778,10 @@ export default function Image( { | |||
| ); | |||
| } | |||
|
|
|||
| if ( ! url && ! temporaryURL ) { | |||
| return sizeControls; | |||
There was a problem hiding this comment.
Do we need a new sizeControls const or can we reuse controls?
There was a problem hiding this comment.
controls includes caption and I think setting caption on placeholder is not expected.
There was a problem hiding this comment.
Could we update controls to extend sizeControls?
There was a problem hiding this comment.
Sure we could add some internal getter for controls that supports an include caption arg but why tho 😁 - it's not something really reused, it's a one time need in terms of the block's codebase.
ajlende
requested changes
Sep 6, 2023
Comment on lines
411
to
415
| <ResolutionTool | ||
| value={ sizeSlug } | ||
| onChange={ updateImage } | ||
| options={ imageSizeOptions } | ||
| /> |
There was a problem hiding this comment.
I don't think the ResolutionTool is needed if you don't have an image to set the resolution of.
Comment on lines
382
to
410
| <DimensionsTool | ||
| value={ { width, height, scale, aspectRatio } } | ||
| onChange={ ( { | ||
| width: newWidth, | ||
| height: newHeight, | ||
| scale: newScale, | ||
| aspectRatio: newAspectRatio, | ||
| } ) => { | ||
| // Rebuilding the object forces setting `undefined` | ||
| // for values that are removed since setAttributes | ||
| // doesn't do anything with keys that aren't set. | ||
| setAttributes( { | ||
| // CSS includes `height: auto`, but we need | ||
| // `width: auto` to fix the aspect ratio when | ||
| // only height is set due to the width and | ||
| // height attributes set via the server. | ||
| width: | ||
| ! newWidth && newHeight ? 'auto' : newWidth, | ||
| height: newHeight, | ||
| scale: newScale, | ||
| aspectRatio: newAspectRatio, | ||
| } ); | ||
| } } | ||
| defaultScale="cover" | ||
| defaultAspectRatio="auto" | ||
| scaleOptions={ scaleOptions } | ||
| unitsOptions={ dimensionsUnitsOptions } | ||
| /> | ||
| ) } |
There was a problem hiding this comment.
This bit is being reused in controls. May as well make it another internal variable that gets used in both places so changes to one don't go missing on the other by accident.
|
Very excited to see this coming along! This will be a huge win for patterns in particular. Tested this and my only point of feedback is around the placement of the placeholder text. Right now, when selecting aspect ratio sizes like the Tall option, the options seem to disappear off the page: 
Here's a recording: aspect.ratio.movFinally, I noticed that for changing the sizing to be really small, the options are cut off in an inelegant way. Definitely edge case but wanted to note as I remember this coming up previously with columns #17721: 
|
|
Thanks for the testing @annezazu 🎉 For the placeholder actions going offscreen maybe I can align them top left instead of center left. For the "very small" edge case I have no idea what to try. |
Can we make the placeholder scrollable? |
I added a commit for this. |
Added a commit for this. |
|
I also did the refactor, so I think this is ready for another review. |
|
Here's the likely experience, within patterns: I think we can follow-up with ideas on improving the active placeholder, maybe make it simpler: 
|
draganescu
commented
Sep 11, 2023
There was a problem hiding this comment.
I think @scruffian 's changes address all the feedback but I won't approve my own PR 😆
This was referenced Sep 13, 2023
38 tasks
36 tasks
12 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?
Closes #48081
Why?
To make placeholders better for layout construction.
How?
Testing Instructions
Testing Instructions for Keyboard
N/A
Screenshots or screencast
placeholder-aspect-ratio.mp4