Background: add background attachment to top level styles #61382
Conversation
ramonjd
added
[Type] Enhancement
|
Size Change: +128 B (+0.01%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
add/background-attachment-block-supports
branch
from
02417ad
to
2bdb93b
Compare
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/background.php ❔ lib/class-wp-theme-json-gutenberg.php ❔ packages/style-engine/class-wp-style-engine.php |
ramonjd
force-pushed
the
add/background-attachment-block-supports
branch
from
2bdb93b
to
9c66d16
Compare
ramonjd
force-pushed
the
add/background-attachment-block-supports
branch
from
9c66d16
to
81399fe
Compare
ramonjd
requested review from
tellthemachines,
spacedmonkey and
ellatrix
as code owners
|
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. |
| @@ -552,6 +564,14 @@ function BackgroundSizeControls( { | |||
| value={ backgroundPositionToCoords( positionValue ) } | |||
| onChange={ updateBackgroundPosition } | |||
| /> | |||
| <ToggleControl | |||
There was a problem hiding this comment.
I've put no thought put into this location or the actual control. Could you tell? 🤣
It does affect the height of the popover, so maybe there's some optimization required.
Elsewhere I've tried using a select dropdown for the size options, and having it site to the side, e.g.,
It's a bit of a control salad 😄
The problem is that translations can and do push out the copy.
I wonder if an accordion or something similar might help to use the space more evenly?
The help text for this toggle could be removed as well.
|
Grouping the toggle with the position UI makes sense to me. But we should probably align more closely with the equivalent control in the Cover block, at least initially: 
Specifically when the toggle is off, Obviously this is separate, but one thing we could try to streamline the panel a bit is hide the position inputs behind a 
|
G ood one. Will do.
Yeah, that sounds like a worth experiment. Thanks for the idea! |
ramonjd
force-pushed
the
add/background-attachment-block-supports
branch
from
132a2fb
to
e5a857c
Compare
Just to confirm, something similar to how the font size controls behave: 2024-07-08.11.18.54.mp4 |
|
Similar, but clicking the settings icon would toggle the visibility of the position inputs (additional to the focal point picker). |
ramonjd
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
Ah, do you mean with relative paths from a block style? I could reproduce that. I'd forgotten to pass the resolved theme paths to the component in #60100 I've just pushed a fix.
I think that was related to the bug you found above if I'm not mistaken. Good spotting! That was a copy past face palm disaster. I've updated with your suggested change. Thanks! I also updated the theme.json schema with Thanks for the thorough testing @aaronrobertshaw Feel free to push to this branch and merge if you think it's okay. |
Yes, good point. It should be consistent between block styles and block global styles. |
There was a problem hiding this comment.
Thanks for iterating on this one @ramonjd 👍
I can confirm the two issues I noted in my last review have been fixed and this is testing nicely for me.
While testing I noticed that display of the background-attachment control cannot be set via theme.json settings. It is also conditional upon background-size being enabled, which might be valid given its display within the background-size popover panel. It's display within that popover should be controllable by theme authors though.
It might be best to address the toggling of the control in a follow-up as I don' t think it needs to block this PR.
Yes, good point. It should be consistent between block styles and block global styles.
The screenshots in my review weren't even between the block inspector and global styles panels. Only the block inspector for Verse vs Group blocks. I'll see what I can do to make them consistent across the board.
@jameskoster would you be okay with the proposed UI update for the background-attachment control to be exposed via a settings toggle, to be explored in a follow-up?
Also, do you think the background-attachment control should be enabled within appearanceTools?
|
I've put up a PR to locate the background image panel consistently: #63551 |
Yes, but to be clear, only the X/Y position need to be hidden since they're effectively duplicative of the focal point picker. The control for fixing the bg should probably be permanently visible. |
…61382) Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
|
I've started looking into adding a toggle to the The @ciampo or @mirka can you share any thoughts on the best approach to achieving this? Does it warrant a v2 of the component? |
|
@aaronrobertshaw would something like this work, or am I misunderstanding the request? |
|
I wonder if it's time to add a |
Thanks, that part of this is straightforward. The problem is we need a button within the
Given how widespread the use of |
I haven't looked too deeply at the FocalPointPicker component, but would it be acceptable to add a prop that would render the "toggling version", e.g., allowing hide/unhide of FocalPointPickerControls? I'm not great at naming, except maybe for small household pets... <FocalPointPicker pointControls="toggle" /> { /* maybe "hidden" as well? */ } |
@ramonjd from what I understand, the tricky part is not hiding/showing the controls, but to add an interactive control, next to the label, that the user can use to toggle such input controls on and off. Currently, given how the component works, that is only possible by adding such toggle externally to the component — folks are wondering if we could do better. |
|
I'm ok with adding a |
What?
Adds a toggle to allow background images to scroll with the page, or remain fixed in position.
Part of:
Core backport PR:
Why?
To increase the possibilities of background images.
How?
Adding support for
background-attachment.TODO
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Kapture.2024-07-08.at.11.52.09.mp4