-
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
Add aspect ratio presets support via theme.json #47271
Conversation
Could we just go with |
Flaky tests detected in 18c2045. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8923035693
|
@mtias I believe We can drop |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
6938ca4
to
c1f53b4
Compare
array( | ||
'path' => array( 'dimensions', 'aspectRatios' ), | ||
'prevent_override' => array( 'dimensions', 'defaultAspectRatios' ), | ||
'use_default_names' => false, | ||
'value_key' => 'ratio', | ||
'css_vars' => '--wp--preset--aspect-ratio--$slug', | ||
'classes' => array(), | ||
'properties' => array( 'aspect-ratio' ), | ||
), |
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 wanted to add just the settings for the image block aspect ratio tool in this PR, but it seems that the UI is coupled to the preset generation. So, even though this generated CSS isn't used, it had to be added to get the UI to work.
I don't think it's too big of a deal since we would add those presets for the aspect-ratio
property anyway for the featured image in a follow-up, but it certainly surprised me that the code was coupled this way.
Size Change: -61 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I would love to see this in an upcoming release. Unless I'm missing it, we cannot add a custom aspect ratio via theme.json. |
packages/block-editor/src/components/dimensions-tool/aspect-ratio-tool.js
Outdated
Show resolved
Hide resolved
@@ -397,8 +407,10 @@ class WP_Theme_JSON_Gutenberg { | |||
), | |||
'custom' => null, | |||
'dimensions' => array( | |||
'aspectRatio' => null, | |||
'minHeight' => null, | |||
'aspectRatio' => null, |
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.
Unrelated: I noticed that you can't disable aspect ration for Image block using this setting. cc @draganescu
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.
Was it supposed to be 😄 What is the expected behaviour here:
- does setting dimensions ≻ aspect ratio ≻ null disable the aspect ratio tools somewhere else?
- should it disappear from the UI of the image block? It does not appear in styles, but in settings, whereas for the cover block the UI appears in styles not in settings. What's preffered?
- should it be null or false? When I set theme.json ≻ settings ≻ dimensions ≻ aspectRatio the editor says it should be bool
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.
The initial null
value is how theme json defines flag settings, and it's not important at the moment.
Setting it to false
disables the aspect ratio UI for blocks. You can test it with the Cover block. I think behavior should be consistent across the blocks that use aspect ratio control.
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.
Yes, indeed setting to false hides this dimension tool from the cover block but it does not hide it from the image block. The reason is that the cover block uses the supports while the image block plops the control in the inspector.
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.
Yes. I'm aware of that. Still, the behavior should be consistent.
@fabiankaegy, do you mind creating an issue for the Image and Featured Image block follow-ups we discussed in Slack?
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.
Yes. I'm aware of that.
I knew that, you told me where to look! 😂 I mentioned here for archaeology reasons.
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.
Created a follow up issue for the aspect ratio block support cleanup: #61432
@ajlende wanted to tag you specifically here for feedback since @oandregal mentioned you recently worked on this area and also you were one of the earlier contributors to this PR before I took it over :) |
Did some user testing on this branch and was able to get through all of the instructions testing custom & default aspect ratios across multiple block themes. |
One note about the testing instructions. Should it list the specific blocks that use aspect ratios? Here's what I tested but I may have missed some:
|
@bacoords I'll update the testing instructions to be a little more concise. But you are correct those are all the instances. The one thing to note it that for the image block it shows in two places. The sidebar setting for the aspect ratio and then also in the croping tools in the sidebar |
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.
Let's merge this. I don't see any blockers here; we can handle feature unification in follow-ups.
I just created a sync PR on WordPress Develop for the PHP updates to the theme.json class https://core.trac.wordpress.org/ticket/61137 |
Internationalization needs to be fixed before this lands in core. See #61767 |
@swissspidy thanks for calling this out. I have created #61774 to fix this here in Gutenberg and also updated my PR in core to include the fix. Question for you. These strings were previously already present in core. Just hardcoded in JS and piped through |
They will be marked as fuzzy because now they have proper context (_x vs __) |
Dev Note DraftAdd custom Aspect Ratio presets through
|
What?
This PR adds support for aspect ratio presets via theme.json. It adds by default the same presets that are available for the crop utilities in the image block.
Why?
Part of #38990
How?
Bootstraps the presets via the theme.json class.
Testing Instructions
settings.dimensions.aspectRatios
) and verify they appearsettings.dimensions.defaultAspectRatios
->false
and verify they do not appearSample `theme.json`
Testing Instructions for Keyboard
N/A, backend only change.
Screenshots or screencast
CleanShot.2024-05-02.at.11.35.18.mp4