Background images: add defaults for background size #62046
Conversation
ramonjd
added
[Type] Enhancement
|
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: +75 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
update/background-image-controls-size-defaults
branch
from
3918fd7
to
0f43362
Compare
|
Thanks for keeping on this, and sweating the details. This is rapidly improving. This PR solves the issue of background images by default being low resolution. But reading up on the properties, and the back compat conversations, I wonder: is it possible to unset this default value if you wanted no value, i.e. |
Good question! I did a quick video and added it to the description - the video suggests that yes, it's possible, but only via clearing the input field. I haven't tested it very thoroughly so I could be off track. |
|
Thanks for the video. Bouncing back from here, I'm really wondering what a good default is. I briefly misremembered "Cover" as behaving how "Contain" with repeat did, which I thought was the preferable one, turns out that's not the case. So thinking between Tile and Contain as defaults, there's no clear win. Tile by default If we add:
That would give us this:
That would be a good experience when you add a large image, and you'd be able to configure it from there. But if you add a low-resolution image with the specific intent to tile it, you'd want to add a pixel value that's half the dimensions of the image itself, to ensure the 2x resolution. I.e. a 100x100 tiled background should be 50px in background-size, and in fact the percentage would make it fill the screen, behave like the aforementioned Contain + repeat that I mentioned. Contain by default As noted, this would provide a good experience if you add a large landscape image. But it would fall apart if you wanted a small tiled background, or used a portrait image instead. Default values For specificity reasons and to not add technical debt, any default extra CSS we output, we have to be very careful. It may provide a better one-click default experience, but it comes at the cost of adding a tiny amount of extra burden when that default is not what you want. To that end, and because of the above test cases, it's not clear that adding a default
Best choice? At this point, it's not clear to me which value is the best to default to. It's probably either contain + repeat, or tile, but without the added default background-size property after all. The only thing that seems clearer after this exercise is that it may be good to add a default background-position, e.g. this: This should center/top align the background image, which when tiled would go well with a centered layout, which is arguably the most common one. Since the Cover block has 50%/50% values by default, it doesn't seem unreasonable to me to add defaults here as well:
So: maybe we keep the tile, but add 50%/0% background position values. What do you think? |
If we apply that only when a user adds an image, that sounds good to try to me! From what you've described, of all the available options, this gets as a little closer to pleasing defaults without boxing us in too much, IMO |
|
Thanks for all the testing @jasmussen 🙇🏻 I'll play around in this PR later in the week. |
ramonjd
force-pushed
the
update/background-image-controls-size-defaults
branch
from
6699665
to
89f91b1
Compare
|
|
||
| if ( next === 'contain' ) { | ||
| nextRepeat = 'no-repeat'; | ||
| nextPosition = undefined; |
There was a problem hiding this comment.
Setting to undefined between toggles so that "50% 0" doesn't persist.
This means however, that any user-defined position values are lost as well.
I'm wondering if we should save the value in local state and only reset when a default value is detected, e.g, "50% 0"
There was a problem hiding this comment.
IMO it's fine to reset the position when switching between sizes as you're kind of resetting how the image will appear anyway. Could be a follow-up PR if we think it's needed after all?
There was a problem hiding this comment.
This is testing nicely for me when switching between background sizes on an already uploaded image. However, if I go to add an image from scratch, the default isn't added when the image is attached. I'm wondering if we also need to add a line somewhere around here in onSelectMedia:
I.e. check if style?.background?.backgroundSize is unset, and if so, set style.background.backgroundPosition to 50% 0?
What do you think? I'm not sure if we've added other values when media is selected before, so wasn't sure if there's any potential edge cases to consider 🤔
|
Tested this branch per the review, but I'm not seeing the defaults added when I add a background image:
|
|
Snap! Joen and I noticed the same thing at the same time 😄 |
Code-wise it's looking good to me. We can either land as-is, and add the behaviour when the image is first uploaded in a separate PR, or hold off on merging until that's added in. Whichever you prefer, @ramonjd! |
|
Thanks for the speedy reviews @jasmussen and @andrewserong I'll aim to add the missing functionality in this PR. Should be a one/two liner I think. Cheers! |
Okay! I've added the change and updated the test instructions. The default of But if the user has already set a position and not toggled from "Tile", the user value is preserved. Thanks again! 2024-05-30.17.10.43.mp4 |
|
Thanks for catching that final feature bug. I'll merge and we can carry onwards and upwards! |
… for uploaded images, also saving the last known value + unit value so it can be restored between toggling
… we default to 50% every time the user switches to auto? What about after they deliberately clear the input? Roll back defaultValues prop - unnecessary
…the database, and only then use `50%` as the default for Tile
…ault background size of 50%, and introduce a default background position of "50% 0"
… images via the media library.
ramonjd
force-pushed
the
update/background-image-controls-size-defaults
branch
from
8398165
to
f2c6e31
Compare
|
Flaky tests detected in f2c6e31. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9299267868
|
|
Nice one, thanks for fixing that up! 🎉 |
Sets a default background position of 50% 0 for media library images. Sets a placeholder for the background image size input control of "Auto" Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
Sets a default background position of 50% 0 for media library images. Sets a placeholder for the background image size input control of "Auto" Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
What?
See: #61928 (comment)
This PR:
"50% 0"for uploaded images when background size is toggled to "Tile"2024-05-30.14.44.44.mp4
Why?
To increase the chance that the image's focus point is visible.
Testing Instructions
Finally, to ensure that no default background position is set for theme.json-defined background images, set a background image in theme.json and toggle between the size settings in the site editor.
2024-05-30.14.52.04.mp4