-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Dimensions: Add Aspect Ratio block support #56897
Changes from all commits
15caba6
280dda5
5008b52
2748fd0
aed99ad
d1ce660
176860a
bbc6868
96daf32
051bb7f
ef7149d
2c9107d
39873bf
7e14457
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -211,6 +211,7 @@ class WP_Theme_JSON_Gutenberg { | |
* @var array | ||
*/ | ||
const PROPERTIES_METADATA = array( | ||
'aspect-ratio' => array( 'dimensions', 'aspectRatio' ), | ||
'background' => array( 'color', 'gradient' ), | ||
'background-color' => array( 'color', 'background' ), | ||
'border-radius' => array( 'border', 'radius' ), | ||
|
@@ -381,7 +382,8 @@ class WP_Theme_JSON_Gutenberg { | |
), | ||
'custom' => null, | ||
'dimensions' => array( | ||
'minHeight' => null, | ||
'aspectRatio' => null, | ||
'minHeight' => null, | ||
), | ||
'layout' => array( | ||
'contentSize' => null, | ||
|
@@ -486,7 +488,8 @@ class WP_Theme_JSON_Gutenberg { | |
'text' => null, | ||
), | ||
'dimensions' => array( | ||
'minHeight' => null, | ||
'aspectRatio' => null, | ||
'minHeight' => null, | ||
), | ||
'filter' => array( | ||
'duotone' => null, | ||
|
@@ -661,6 +664,7 @@ public static function get_element_class_name( $element ) { | |
array( 'color', 'heading' ), | ||
array( 'color', 'button' ), | ||
array( 'color', 'caption' ), | ||
array( 'dimensions', 'aspectRatio' ), | ||
array( 'dimensions', 'minHeight' ), | ||
// BEGIN EXPERIMENTAL. | ||
// Allow `position.fixed` to be opted-in by default. | ||
|
@@ -2093,6 +2097,15 @@ protected static function compute_style_properties( $styles, $settings = array() | |
$value = gutenberg_get_typography_font_size_value( array( 'size' => $value ) ); | ||
} | ||
|
||
if ( 'aspect-ratio' === $css_property ) { | ||
// For aspect ratio to work, other dimensions rules must be unset. | ||
// This ensures that a fixed height does not override the aspect ratio. | ||
$declarations[] = array( | ||
'name' => 'min-height', | ||
'value' => 'unset', | ||
); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like min-height is already getting removed when aspect-ratio is set in global styles, so do we still need this here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes — we need the |
||
$declarations[] = array( | ||
'name' => $css_property, | ||
'value' => $value, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly interested... so
$tags->add_class( 'a-class b-class' )
should work too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment I'm calling
explode
so that we add them one-by-one... I looked up theadd_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 simpleradd_class
with the full single string, though 🙂Thanks for giving this PR an early look! 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a try and it works 😄 No biggie, I was just wondering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one! I can switch it over 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂