Background image: size controls should show when an image is set #61388
Conversation
ramonjd
added
[Type] Bug
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -257 B (-0.01%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for following-up, but I think this might be looking to fix something different to what I had in mind in my review comment:
One subtle thing I noticed in the UI is that it doesn't expose the size controls by default when the theme.json sets an image. Should it, do you think?
What I had in mind was that it seemed off to me when adding an image in global styles, and for the size controls not be shown by default after adding an image. Apologies if I missed the discussion where this was changed!
Edit: alternately I may have messed up my testing environment here, which is another possibility, as I'm running into a number of unrelated errors locally. I'll give this another test tomorrow 🙂
| backgroundSize: | ||
| !! style?.background?.backgroundImage && | ||
| !! inheritedStyle?.background?.backgroundImage, | ||
| backgroundSize: false, |
There was a problem hiding this comment.
This means that the position controls aren't showing if a background image is set, with no size controls. Was the intention for them to be hidden? Apologies if I missed some of the evolution of the design discussion, but I thought the behaviour in global styles was intended that if you upload or add an image (or if an image is set via theme.json) that the size controls would always be visible.
That way when folks go to add an image, or to this part of the UI, the controls to adjust the background image are easily accessible, with the only time the controls are hidden being when there is no image.
There was a problem hiding this comment.
Apologies if I missed some of the evolution of the design discussion, but I thought the behaviour in global styles was intended that if you upload or add an image (or if an image is set via theme.json) that the size controls would always be visible.
That was how things started out, but it was changed to be the same UX/UI as block supports.
See: #60264 (comment)
No, you're seeing things right. I was both assuming knowledge and reading your sentence wrong, sorry!! Since this PR and this conversation, I've tried to keep the UX/UI the same between global styles and block supports.
So I immediately jumped to a conclusion when you mentioned that the size controls weren't showing by default and my brain replaced "image" with "size". 😅 If you think we should revert again to what you expected, we could open the design discussion up again? |
ramonjd
removed
the
Backport to WP Beta/RC
Thanks for clarifying!
Sorry to be a pain, but yes, I'd like to hear more from @jameskoster or @richtabor on this one. To me, having the size controls hidden by default feels inconsistent with other global styles controls. For spacing and typography, for example, at the block level in the post editor, we often keep things hidden by default, but in global styles they're all exposed by default. When someone goes to add a site-wide background image, I think it's quite likely they'll want to tweak the image in some way, so I'd preference showing size controls by default once an image is added in global styles. Totally happy for us to go in a different direction if designers prefer, but I think it's worth double checking again, if that's okay 🙂 |
No worries. Whichever way we go, it's an easy change 👍🏻 |
Agree with this, the behavior on trunk for the 'site' background image in global styles feels right to me. I know it's separate, but I still think putting the Size (and maybe other) settings in a popover is worth a try, like this old mockup: 
That would essentially solve this issue. Also; these settings currently take up way too much space in the Inspector given they're basically set-and-forget. I couldn't see if this was tracked in #54336 at all. |
Good stuff, thanks for confirming. I'll update this PR accordingly.
Based on your designs, I have a WIP/experimental/explosive PR here, which is linked from the tracking issue (under General/not-yet-prioritized tasks). It won't be in WordPress 6.6 as it involves quite a bit of faffing about with ToolsPanel integration, and there are a bunch of unknowns (at least to me) in that area. I plan to pick it up again and investigate further. |
…ent, just like block styles.
|
Okay folks, I've updated this PR to ensure global styles exposes the size controls by default when the theme.json sets an image 🤞🏻 Thanks! |
ramonjd
changed the title
ramonjd
force-pushed
the
update/site-editor-show-background-size-when-value-present
branch
from
7841476
to
00b579d
Compare
| const hasValue = hasBackgroundSizeValue( style ); | ||
| const hasValue = | ||
| hasBackgroundSizeValue( inheritedValue ) || | ||
| hasBackgroundSizeValue( style ); |
There was a problem hiding this comment.
Note to self:
There's an potential design edge case, where theme.json specifies a background-image string value for background.backgroundImage, e.g., url('http://test.org/image.png'), the controls won't display a preview because the value isn't being parsed.
I think this is correct as we can't possibly parse all possible values, e.g., linear gradients, image urls and multiples of both, but maybe we could pass a dummy image to the position controls at least, e.g., "No preview available" or something, or even disable the position drag controls.
Also, the copy could be changed from "Add background image" to "Update background image" when we detect a string value for background.backgroundImage, e.g.,
const previewLabel =
title ||
( ! style?.background?.backgroundImage?.url &&
typeof inheritedValue?.background?.backgroundImage === 'string'
? __( 'Update background image' )
: title );
packages/block-editor/src/components/global-styles/background-panel.js
Outdated
Show resolved
Hide resolved
|
I appreciate your help @andrewserong and @jameskoster ! |
ramonjd
deleted the
update/site-editor-show-background-size-when-value-present
branch
Part of
What?
In the site editor, show the background size panel by default if an image value is present. 🎉
Props to @andrewserong for noticing
Why?
So that the global styles control is consistent with other global styles.
It should be, hence why this PR is labelled as "bug".
How?
Checking for inherited background image value (from theme.json) as well as user-defined style value. If either is present, show the control unless background size is deactivated via theme.json settings.
Testing Instructions
TL;DR In the site editor, for top-level global styles, the background size controls should show where there is an image value.
Background size controls are shown by default
{ "$schema": "../../schemas/json/theme.json", "version": 2, "settings": { "appearanceTools": true }, "styles": { "background": { "backgroundImage": { "url": "https://images.pexels.com/photos/22484288/pexels-photo-22484288/free-photo-of-the-circular-stone-terraces-of-the-inca-ruins.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=2" } } } }Background size controls are displayed with theme.json value
{ "$schema": "../../schemas/json/theme.json", "version": 2, "settings": { "appearanceTools": true }, "styles": { "background": { "backgroundImage": { "url": "https://images.pexels.com/photos/22484288/pexels-photo-22484288/free-photo-of-the-circular-stone-terraces-of-the-inca-ruins.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=2" }, "backgroundSize": "contain" } } }Background size controls are completely disabled
{ "$schema": "../../schemas/json/theme.json", "version": 2, "settings": { "appearanceTools": true, "background": { "backgroundSize": false } }, "styles": { "background": { "backgroundImage": { "url": "https://images.pexels.com/photos/22484288/pexels-photo-22484288/free-photo-of-the-circular-stone-terraces-of-the-inca-ruins.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=2" }, "backgroundSize": "contain" } } }Also check that things work with your own site-wide background image, uploaded in global styles
Thank you for testing this PR. 🎖️
2024-05-08.12.00.11.mp4