Background images: ensure appropriate default values #64192
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramonjd
added
[Type] Enhancement
71 tasks
|
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. |
4 tasks
ramonjd
force-pushed
the
fix/background-image-user-uploaded-defaults
branch
from
22926ae to
d4c9ae9
Compare
ramonjd
commented
Aug 2, 2024
| * For user-uploaded images at the block level, assign defaults. | ||
| * Matches defaults applied in the editor and in block supports: background.php. | ||
| */ | ||
| if ( static::ROOT_BLOCK_SELECTOR !== $selector && ! empty( $styles['background']['backgroundImage']['id'] ) ) { |
There was a problem hiding this comment.
This is the same as the assignments performed in background.php
Thinking this could be abstracted to a function either in background.php, or the style engine?
What do you reckon @andrewserong ?
There was a problem hiding this comment.
or the style engine?
Well, I think the style engine shouldn't provide defaults willy-nilly, but maybe it could "register" defaults or accepts them in the options argument.
Maybe a separate function is better here, e.g.,
gutenberg_style_engine_get_styles( array( 'background' => gutenberg_apply_style_defaults( $styles['background'] ) ) )
There was a problem hiding this comment.
To be honest, I think I'd go with the duplication for now or until we have another block support that needs default values in this way, as what we're doing with background image seems fairly unique right now. Are there other use cases for gutenberg_apply_style_defaults we know of in the near-term?
If not, I'd lean a little toward going with the duplication, especially since it's only ~5 lines or so, and revisit if and when we have other defaults to apply. What do you think?
There was a problem hiding this comment.
Are there other use cases for gutenberg_apply_style_defaults we know of in the near-term?
Not that I know.
Thanks for the feedback. I agree with your reasoning. I'll leave it for now then. 🙇🏻
| inheritedValue?.background?.backgroundSize; | ||
| const positionValue = | ||
| style?.background?.backgroundPosition || | ||
| inheritedValue?.background?.backgroundPosition; |
There was a problem hiding this comment.
This ensures inherited values (from global styles) are taken into account.
There was a problem hiding this comment.
Whoops, I think I introduced a bug here. We don't want the inherited styles to creep into the block supports styles - see the onChange below.
Note to self for tomorrow:
To test, insert a group block that has global styles. Replace the image with an uploaded one.
The styles from global styles should apply, but not be saved to the inline CSS.
The easiest thing to do would be to reset the styles once an image is uploaded, but that would ignore any theme styles. 🤔
|
Size Change: +16 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
- ensure that global styles background image blocks with user-uploaded images receive default values
- ensure that global styles background image blocks with user-uploaded images receive default values
ramonjd
force-pushed
the
fix/background-image-user-uploaded-defaults
branch
from
d4c9ae9 to
5fe858e
Compare
andrewserong
approved these changes
Aug 7, 2024
There was a problem hiding this comment.
This is all testing well for me 👍
✅ No defaults are applied in the editor or site frontend when using theme.json-based background images (site wide and block level in global styles)
✅ No defaults are applied for user uploaded site wide background images
✅ Defaults are applied correctly for user uploaded block-level background images in global styles
✅ Defaults are applied correctly for user uploaded block instance background images
LGTM! 🚀
As an aside, while testing in smaller viewports, I noticed that the Popover is looking quite tall vertically again now that we have the Fixed background option, and when uploading taller images. It's fine in a big desktop, but at smaller laptop screensizes, I'm noticing the popover can get a scrollbar:
| No scrollbar | Taller image: scrollbar |
|---|---|
 |
 |
Not for this PR, but separately, would it be worth tweaking either the Fixed control (reduce the help text, or how much space it uses, possibly), or further tweaking the max height on the focal point picker? Apologies if this has already been discussed elsewhere!
|
Thanks for testing so thoroughly!
I think there are comments some where that allude to tweaking it all, but there's nothing concrete. Definitely worth a revisit 🙇🏻 |
1 task
14 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
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.
Why set defaults only for uploaded images?
Setting defaults changes the way the background-image appears.
Images added by the user in the editor receive defaults to increase the chances that they look "okay" in the editor immediately.
We should respect, as much as possible, the values in theme.json, that includes values that are not set. The assumption is theme developers explicitly add and omit values. If Gutenberg were to add default to theme.json styles, it would undermine that assumption.
How?
Check for uploaded images, all of which have an id, and apply defaults for block and not site-wide images.
Testing Instructions
A list of test scenarios:
1. Background images defined in theme.json
No defaults should be applied in the editor or frontend. That is, whatever styles you see in theme.json will be reflected in the CSS.
The controls display the image and the values. The following screenshot displays controls where only
backgroundImagehas been defined in theme.json. "Tile" and "Repeat" are selected since these are the CSS defaults where no declaration exists.3. Background images uploaded to global styles (site editor)
For site wide images, no defaults are applied.
For blocks, background images uploaded to global styles will receive the defaults, e.g., "cover". Accordingly, if the theme.json block style specifies a background size of "contain", the background position of "center" should be applied.
Try selecting Styles > Blocks > Quote and uploading a new image to the background image. Save.
Then, in a post, add a Quote block. Check that the defaults are applied in the editor and frontend.
Make sure you can overwrite everything at the block level.
All block-level (block supports) background image styles will receive the defaults, e.g., "cover".
This means that clicking on a block, e.g., Quote, Verse, Group, and uploading/selecting an image from the media library, you'll see a default background size of "cover".
Example theme.json
{ "$schema": "../../schemas/json/theme.json", "version": 3, "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" } }, "blocks": { "core/verse": { "background": { "backgroundImage": { "url": "https://images.pexels.com/photos/105819/pexels-photo-105819.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=" }, "backgroundSize": "contain" } }, "core/quote": { "background": { "backgroundImage": { "url": "https://images.pexels.com/photos/105819/pexels-photo-105819.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=" } }, "dimensions": { "minHeight": "100px" } } } } }Example block HTML