Dimensions: Add Aspect Ratio block support #56897
Conversation
andrewserong
added
[Block] Cover
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/dimensions.php ❔ lib/class-wp-theme-json-gutenberg.php |
|
Size Change: +539 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2c9107d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7579532565
|
andrewserong
force-pushed
the
try/dimensions-aspect-ratio-block-support
branch
from
2525ba0 to
bf39d96
Compare
|
Note to self: for some reason the Also, I had to add a |
| // May be set to undefined, so check if the property is set! | ||
| if ( | ||
| propsA?.hasOwnProperty( 'className' ) && | ||
| propsB?.hasOwnProperty( 'className' ) | ||
| ) { | ||
| newProps.className = classnames( propsA.className, propsB.className ); | ||
| } | ||
| if ( propsA?.style && propsB?.style ) { | ||
|
|
||
| if ( | ||
| propsA?.hasOwnProperty( 'style' ) && | ||
| propsB?.hasOwnProperty( 'style' ) | ||
| ) { |
There was a problem hiding this comment.
This merges in a fix that will land in #56912 to ensure classnames are output correctly. Just including it here for now so that this PR will work properly in manual testing 🙂
| &:where(.has-aspect-ratio) { | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
We'll likely need to put this rule somewhere else as I think we'll always want this applied when .has-aspect-ratio is active.
andrewserong
requested review from
jasmussen,
apeatling,
ramonjd and
tellthemachines
|
|
||
| if ( ! empty( $styles['classnames'] ) ) { | ||
| foreach ( explode( ' ', $styles['classnames'] ) as $class_name ) { | ||
| $tags->add_class( $class_name ); |
There was a problem hiding this comment.
Mainly interested... so $tags->add_class( 'a-class b-class' ) should work too?
There was a problem hiding this comment.
At the moment I'm calling explode so that we add them one-by-one... I looked up the add_class method and it does look like we should be able to call it with a full classname string that contains multiple classes, I just wasn't sure if it's guaranteed to 🤔. Happy to switch it over to the simpler add_class with the full single string, though 🙂
Thanks for giving this PR an early look! 🙇
There was a problem hiding this comment.
I gave it a try and it works 😄 No biggie, I was just wondering.
There was a problem hiding this comment.
Nice one! I can switch it over 😀
There was a problem hiding this comment.
Yeah if it's not explicitly indicated in the docs it might not be officially supported even though it currently works. We were using similar logic to fetch a bunch of classes from an element in one go in the layout support, but this change broke that ability. The refactor of layout in #54075 to fix that has more info.
There was a problem hiding this comment.
Agree - might be a nice feature though to have an add_classes method that accepts an array or something.
There was a problem hiding this comment.
Thanks folks, good points! Related to what you've linked to in #54075, I just did a quick test of what happens if the class string includes multiple classes, and it seems it gets treated as a single classname so the de-duping logic in add_class gets skipped, resulting in potential duplicates:
I'll stick with the foreach for now 🙂
|
Awesome stuff! I know it's early days, but it's working a treat so far. Looking forward to testing it again 2023-12-12.16.29.36.mp4 |
|
This is fantastic. Functionally it is working exactly as I had hoped/expected. Here's a GIF showing the new aspect ratio control, with frontend preview, fully functional:
The main question is where to put this control, where in this PR you've put it in the Dimensions panel:
This is arguably the correct way to place it, and follows a pattern established by the Featured Image block as well:
The odd one out is the Image block, which puts it in Settings:
A soft ping to @WordPress/gutenberg-design for input here; what do we see as the unified end-state for this control? Would it make sense to (separately) move the dimensions controls from the Image settings tab to the style tab, and what would then happen to the Alt text? On a separate note, I would expect the Cover block to eventually also get width controls on every block (#25973), so in principle we can move towars this design for the Cover block height control even today:
We can just call it "Max height" instead of "Height". "Maximum height of Cover" is unusually verbose and slightly redundant. Not a blocker here, but simply thinking ahead. Nice work, thank you! |
That feels like an important consideration. There's already something a little awkward about the contradiction that can occur between min-height and aspect ratio. If you can define static height / width, how does the interplay with aspect ratio work? For this PR specifically, is it intended that content becomes invisible if it is too 'tall' for the container? Should there be |
If you have both an aspect ratio, and a min-height, the aspect ratio will define the height until it reaches the min-height, which is what I'd expect.
What are you seeing here? |
That's not how it works though? If I set aspect ratio to 1:1 and min-height to 1000000px then the aspect ratio will never be respected because there's not enough horizontal space to do so.
If the height of the contents inside the Cover greater than the aspect ratio allows for the Cover height then the content gets cut off at the top and bottom. Hopefully this video explains better: cover.mp4 |
Right, apologies, I miswrote my sentence, I meant "reaches" in the scaling down kind of way. A different way to say it is: min-height is always min-height, so if the height as inferred by the aspect ratio is smaller than the min-height, the min-height will serve s the floor value. Which is what I'd expect, it remains the minimum value.
Right, yes, thank you for clarifying. Honestly I think we can explore an overflow design tool as a separate thing, and leave it to an editorial responsibility to ensure things look good here. |
andrewserong
force-pushed
the
try/dimensions-aspect-ratio-block-support
branch
from
c52cd77 to
c071b77
Compare
andrewserong
changed the title
|
I just cherry-picked this PR to the release/17.6 branch to get it included in the next release: bcc44ac |
cbravobernal
removed
the
Backport to Gutenberg RC
|
Let's add this to the #57959 issue as it requires some manual php backports and please consider opening a core trac ticket and backport. Thank you. |
* Try: Dimensions Aspect Ratio block support * Try server-rendering output * Try outputting has-aspect-ratio classname * Ensure block support is checked properly * Try adding global styles support * Update docs, add appropriate checks * Try unsetting minHeight * Try unsetting min-height when applying aspect ratio and vice versa * Allow Cover block to expand when the content extends beyond the boundaries of the aspect-ratio * Hide cover block resize handle when aspect ratio is set, clear aspect ratio when updating cover block min height * Clear aspect-ratio if minHeight is set on the site frontend * Attempt to get unset rules playing nicely in the editor with global styles * Hide aspectRatio control from global styles for the group block * Skip output of has-aspect-ratio if value isn't set
I'm noticing the aspect ratio support still on the group block:
|
|
G. O. L. D. |
|
I noticed this PR was merged after I raised the PHP Sync Tracking Issue for WP 6.5 and has changed PHP files that may need backporting to WP Core. I've now added it to the Tracking Issue. |
This will be a WordPress 6.5 feature as it only landed in Gutenberg 17.6. See: WordPress/gutenberg#56897
|
Thanks folks, I'll work on backports this week.
@richtabor it's only hidden at the global styles level for the Group block. At the individual block level it's supported as it's still useful there. The main thing we wanted to avoid was folks accidentally setting all Group blocks to a particular aspect ratio and then breaking site layouts. |
Update: I've opened a trac ticket and a backport PR over here: WordPress/wordpress-develop#5963, and updated the backports tracking issue with links to those two 🙂 |
How is aspect ratio on a group block helpful? If we don't have the cover block's position controls, it feels unresponsive if the group block has more than a couple blocks, or is fullwidth. I think there's too many variables where a group block is used differently from a cover block, where it doesn't make as much sense to include. aspect-ratio-group.mp4 |
That's a good point. It's hard to use with the Group block as it only really feels at home with a Row or Stack block where the content is small enough for it to be useful and where we have the justification controls. That makes it a bit of a niche feature. Here's an example of using it with the Row variation: 2024-01-30.08.10.08.mp4In its current form, I agree the aspect ratio controls are likely too prominent on the Group block for something that'll likely be used very rarely. I'm in favour of removing support on the Group block for now, and leaving it just on the Cover block for WP 6.5. We can then revisit for subsequent releases if we think it feels missing not having it on the Group block. I have a PR up to remove support on the Group block over in #58414 |
|
Update: #58414 has been merged, so support is only for the Cover block for now. |
youknowriad
added
Backported to WP Core
|
As a result of adding the aspect ratio to the Cover block... |
|
Draft dev note: Aspect ratio block supportIn WordPress 6.5, a new aspect ratio block support has been added, with the Cover block opted-in by default. For themes using the The feature allows users to set an aspect ratio for the Cover block, and is mutually exclusive with Note that themes or blocks that add width or height rules to a block will need to take care in testing compatibility with How to add aspectRatio support to a themeThere are two ways to add support for For themes that wish to have more granular control over which UI tools are enabled, the |
andrewserong
added
the
has dev note
What?
Fixes: #54032
This PR explores adding aspect ratio to the Cover and Group blocks by including it as part of the dimensions block supports.
It supports aspect ratio at the individual block level, and within global styles. Note that the override for the Cover block that switches off its min-height value only works at the individual block level, and not in global styles.
Why?
We currently only have minHeight controls, which doesn't allow for easily creating particular proportions. It'd be helpful for many different kinds of site designs to be able to set container blocks like Cover and Group to a particular aspect ratio.
How it works (and potential limitations)
This PR tries to let the CSS
aspect-ratiowork the way that it works, with minimal interference. To achieve this, and to avoid overflow issues, when anaspect-ratiois set, weunsetany value formin-height. Similarly, if amin-heightis set at the block level, we output anaspect-ratio: unsetrule to override any aspect ratio set in global styles. Theseunsetrules should only be output for blocks that opt-in to aspect ratio, and are not saved to post content.For folks wanting to reduce the width of their container to less than the full width of the container, a good way to do it is to wrap the block in a Row block, and use the child width controls, as the flex-basis rules for fixed width appear to play nicely here.
How?
aspectRatioto dimensions block supportmin-height: unsetrule that gets applied when aspect ratio is in use — this is necessary because setting amin-heightinterferes with aspect-ratio behaviour by not allowing content to expand the area of a block beyond the defined aspect ratio. In order to ensure that longer content flows correctly in narrower viewports, we unsetmin-heightwhen aspect ratio it applied.aspect-ratiorule into post content. This is because the aspect ratio rule depends on other CSS values, so it's safer if we don't serialize the value for now. This also allows us to explore other options in the future for output, like usinghas-4-3-aspect-ratioclassnames that apply the aspect ratio instead of inline styles. And finally, it allows us to addaspect-ratioandunsetrules without having to deprecate any legacy markup.min-height: unsetrule when outputtingaspect-ratio— this is a little hacky, but ensures that rules like the Cover block's defaultmin-heightdon't interfere with the aspect ratio.Testing Instructions
minHeight, too — notice thatminHeightwill clear out any aspect ratio applied.In testing, how do these controls feel to use? Is it close enough to desired behaviour to be useful, or does it feel limiting? Try adding a variety of content within the Group and Cover blocks to get a feel for it, too.
Screenshots or screencast
2023-12-12.12.18.34.mp4