-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Background images: resolve ref and ensure appropriate default values #7137
Background images: resolve ref and ensure appropriate default values #7137
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Could you please create a Trac ticket for this and plop it in the 6.7 milestone? |
e86b5b7
to
dd12193
Compare
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
be97f10
to
a4caf72
Compare
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 for putting this together @ramonjd 👍
While comparing the code changes here with their Gutenberg counterparts, it appears this backport doesn't include all the latest changes that landed in GB. For example, the change to merging background image configurations in theme.json data (WordPress/gutenberg@cb34d72).
Some of the later changes on WordPress/gutenberg#64128 are here, like the unit test update but not others.
I can help out tomorrow with bringing across any missing pieces. I'll also hold off on testing until then.
Thanks for double checking! I'll fix this first thing this morning 🙇🏻 |
6d88660
to
b32f4ae
Compare
I'd left the previous "unset" ref logic (and related test) in there. 🤦🏻 I think it should be pretty close now, but I'm a bit bug-eyed . |
There's one inconsistency between this PR and Gutenberg trunk, where Gutenberg trunk is wrong. I have a sync PR here: This was due to a bit of flip-flopping on features and I forgot to remove it. 🙇🏻 |
…eir appearance. For example, block styles receive a default background size of "cover" in the editor and the frontend. Or, where the background size is "contain" the background position is "center". Defaults have always applied to images uploaded by the user in the editor. The PR brings a bit of consistency to background image styles. Block styles "inherit" values from global styles/theme.json and display the current value (whether the set, inherited or default value) in the editor controls. In relation to default values: - Site wide background images (uploaded or otherwise) do not receive any default values. - Block background images defined in theme.json do not receive any default values. - Block background images that have been uploaded (images with ids) receive background default values.
…ct, but the latter will appear in the UI controls.
…t merged. The image object represents a single unit. We want to prevent "cross contamination"
…e synced background-size: contain;background-attachment: fixed;
45a09be
to
03bf346
Compare
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.
In general, this is testing well for me on the frontend of my test site 👍
This PR will only affect the frontend (PHP).
Consistent with this, the ref
based values are working for me in the post editor but not the site editor (I'm assuming we'll need the packages update before we can test there 👍).
Here's what I've encountered in testing:
✅ Code changes match the final state from the referenced PRs
✅ The ref
-based resolution works as expected
To test defaults, you can add "id": 123 to the background.backgroundImage object. For this case:
❓ For some reason I'm having trouble testing this. I've added "id": 123
to the verse block's background.backgroundImage
object in my local theme.json
file, but on the site frontend I'm not seeing the default cover
background-size rule. Instead I'm only seeing the background image rule:
Are you able to replicate? It's highly possible I've made a mistake in my local testing environment, so just wanted to double check 🙂
Overall, though, this backport looks to cover the relevant bits of code from the linked PRs 👍
My bad @andrewserong I forgot to specify that the image cannot be a relative image - is that what you were testing with? The reason being, relative images won't ever have ids because they're not uploaded to the media library and therefore not saved to the database with an id. In that way "id" is a reserved attribute for uploaded images. Any "id"s added to a background image object with a relative path will be stripped out here: https://github.com/wordpress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-theme-json-resolver.php#L945 Now, that's not intentional, it's just the practical reality 😄 The other side of the coin is that if we support more properties in This should work: "core/verse": {
"background": {
"backgroundImage": {
"url": "http://localhost:8889/wp-content/themes/twentytwentyfour/assets/images/building-exterior.webp",
"id": 123
}
},
"color": {
"text": "white"
},
"dimensions": {
"minHeight": "100px"
}
}, |
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.
This should work:
Ah, wonderful thank you for getting to the bottom of that! That all makes sense to me, I knew I must have been missing something in my local testing but couldn't for the life of me figure out what 😆
Now, that's not intentional, it's just the practical reality 😄
That all sounds reasonable — the defaults logic is really just meant to apply for users adding values via the UI, so I don't see any issues here.
The other side of the coin is that if we support more properties in background.backgroundImage, they'll have to be merged correctly for relative path images.
I can't think of any other circumstances where we'll need to add other properties for relative path images, so I imagine we might not run into any issues there 🤔
After re-testing this looks good to land to me! 🚀
A PR to sync the changes in:
Default values
Block background images have long had "default" values to optimize their appearance.
For example, block styles receive a default background size of "cover" in the editor and the frontend. Or, where the background size is "contain" the background position is "center".
Defaults have always applied to images uploaded by the user in the editor.
The PR brings a bit of consistency to background image styles.
In relation to default values:
Resolving theme.json "ref" values
Add support for ref resolution to "background" style properties.
Testing
This PR will only affect the frontend (PHP).
With the following theme.json applied, check that the "ref" values are correctly resolved.
To test defaults, you can add
"id": 123
to thebackground.backgroundImage
object for non-relative path images:For example:
For this case:
Example theme.json (hot swap for TT4's)
Here is some example block HTML as well
Trac ticket: https://core.trac.wordpress.org/ticket/61858
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.