-
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
Background image: update controls defaults and layout #62000
Conversation
…-wide background image in the editor by settings the value to `background-image: none`
…the label "Fixed" to "Tile", using horizontal layout for image size and repeat, fixing bug where width input is cleared and the size defaults to "cover"
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. |
…defaults' into update/background-image-control-defaults
…ge title or image filename. Where there is no image URL, then show no background image selected
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.
Fast work, big improvement. Bug is fixed, and this is coherent with the Group option as well:
As I argue in this comment, I think we should also still change the default from Tile to Cover. The short summary of why is: until we have background-size and background-position tools to enable 2x resolution, and top/centered as opposed to top/left tiling position, "Tile" as a default is not going to be a good or expected experience.
That's not a blocker, so we can discuss this separately if need be.
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 is testing well for me, too, and adds a nice level of polish. (Great idea @jasmussen to use the name Tile, too, it fits much better than Fixed, and allows for a true fixed option further down the track).
I think we should also still change the default from Tile to Cover ... That's not a blocker, so we can discuss this separately if need be.
On this one, I still feel like Cover for site wide backgrounds as a default risks leading users astray, so IMO a separate PR for switching the defaults would be a good way to go if we're planning to change it, as that way we've got the change clearly documented in its own PR.
This one LGTM! 🚀
I appreciate the test and feedback, folks!
I'm also for leaving it set to "auto" by default, but for different reasons. For theme.json, I think we should be wary of inserting any default values that a user might not expect. My view is that theme.json should primarily represent positive intent, that is, "turn stuff on", rather than override opinionated CMS rules. |
Oh, and the backwards compat "fun" there'd be if folks ever wanted to change/roll back the default. 🤣 |
Thanks for moving fast on this one, also carefully. I did mention that I think Cover may be a better default experience, but as it turns out, I was misremembering aspects of this implementation, which in more in-depth testing reveals that, well, there probably really isn't any good default here, unless we add another bit of bundled default code, which I'll get back to. My instinct was for Cover to solve these issues:
Turns out I was thinking of the behavior of Contain, and even in that case, it only works with Contain + repeat, and then only for landscape images. Here's a case where that works well: But even that falls apart if the image is portrait oriented. At that point it's still top/left aligned, but tiles horizontally. For reference, here's the current default, Tile, which as noted has its own problems: I'll dive back into #62046 with another suggestion, yet another new default! |
"Cover" would be the best default, when used like in good old Customizer as cover the screen (not the whole webpage!) in combination with "background-attachment: fixed" So, if there is no comparable solution to Customizer default, this feature is - in my opinion - not ready for WP 6.6 |
Thanks for the input @burnuser 🙇🏻
Background attachment controls are in the works: But it won't be part of 6.6 unfortunately. I'm working on this stuff as fast as I can 😄 As for the In my opinion, we should be cautious about assuming defaults in theme.json in the absence of any values. In the case of background size, the CSS default is
There's a discussion going on how best background images in block themes can play with the current Customizer tool. The current state is that background images set via the Customizer will take precedence over those set in theme.json due to CSS specificity, so you can continue to use it without any interference from this new feature. |
* Background image: add functionality to remove a theme's default, site-wide background image in the editor by settings the value to `background-image: none` * Background image: harmonize headings, update copy including changing the label "Fixed" to "Tile", using horizontal layout for image size and repeat, fixing bug where width input is cleared and the size defaults to "cover" * Make sure the visually hidden labels are correct - use either the image title or image filename. Where there is no image URL, then show no background image selected Co-authored-by: ramonjd <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: andrewserong <[email protected]>
* Background image: add functionality to remove a theme's default, site-wide background image in the editor by settings the value to `background-image: none` * Background image: harmonize headings, update copy including changing the label "Fixed" to "Tile", using horizontal layout for image size and repeat, fixing bug where width input is cleared and the size defaults to "cover" * Make sure the visually hidden labels are correct - use either the image title or image filename. Where there is no image URL, then show no background image selected Co-authored-by: ramonjd <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: andrewserong <[email protected]>
What?
Maybe resolves #61928
This PR:
Why?
Trying to bring some consistency to the controls.
Testing Instructions
... To come
Screenshots or screencast