Background image support: add background position controls #58592
Conversation
andrewserong
added
[Type] Enhancement
| @@ -367,6 +368,64 @@ function backgroundSizeHelpText( value ) { | |||
| return __( 'Set a fixed width.' ); | |||
| } | |||
|
|
|||
| const coordsToBackgroundPosition = ( value ) => { | |||
There was a problem hiding this comment.
This is just a quick pass at the mapping — if this PR looks okay, I'll fine-tune the logic a bit here and add some tests.
There was a problem hiding this comment.
This particular bit LGTM, but just curious: why do we need to do this mapping? Aren't the percentage values enough?
There was a problem hiding this comment.
Good question: we need to have at least some form of mapping so that we're not storing values like { x: 0.5, y: 0.5 } in the block attributes for style.background.backgroundPosition. For future proofing the feature, I think it's better if that value can be stored in the block delimiter as a real CSS value, rather than a value that's intended for the internal workings of the focal point picker. So at the very least, I think it's better if we can have the support map { x: 0.5, y: 0.5 } to 50% 50%.
In terms of mapping the natural language values like left, center, etc, that's mostly me thinking of what would be good to support for if/when we get to theme.json support where folks might be writing the JSON a bit by hand. It'd be nice to be able to support mapping potentially handwritten values so that they work naturally with this control.
For now, though, we only really need the generic decimal 0.5 > 50% mapping for this to work, so I'm happy to pare it back for this PR.
There was a problem hiding this comment.
In terms of mapping the natural language values like left, center, etc, that's mostly me thinking of what would be good to support for if/when we get to theme.json support where folks might be writing the JSON a bit by hand.
Very good idea!
There was a problem hiding this comment.
For now I've just gone with converting between percentage and coordinate values. There's a bit more nuanced to the multi-value syntax for background position, so I think that'd be better to handle separately in a follow-up code quality PR. For now I've gone with the main use case logic which is background position values that were created using this particular UI control.
|
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 Core SVNIf you're a Core Committer, use this list when committing to GitHub Merge commitsIf 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. |
|
@WordPress/gutenberg-design just a ping to see if the idea in this PR could be viable for 6.5 — I know we're short on time before Beta 1, so I was wondering if it's worth adding the Focal Picker to the background image size controls (just the same as we use in the Cover block) for now, before attempting a redesign of the panel into a popover as raised by @jameskoster back in #57005 (comment) In short: my main question is whether we can (or should) try to get this control in for 6.5 in a simple way, or if it's better to hold off for a more holistic redesign. Happy for any feedback either way! |
|
Size Change: -9.39 kB (-1%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
In trunk, you'll see this when you add the non-default size option to the background panel:
In this branch, you'll instead also see the focal point picker, in the same option:
Given it's non-default, to me this doesn't seem harmful to add. The main issue that's slightly exacerbated is that the similar tools on the Cover block are getting increasingly out of date, living in the settings panel, not using the toolspanel, etc: That could potentially be a related followup, probably not a blocker, and something we need to do regardless. |
|
When the background size is I'm likely missing some nuance, but for the fixed/contain cases, I wonder if a version of the matrix control might be more useful? This one: 
|
|
Love your work. Noticed two things when I gave this a spin.
|
|
Thanks @noisysocks, I missed those on Friday, I'll fix 'em up! 🙂 |
|
Thanks for the reviews, everyone! I've added a couple of tests for part of this, and have fixed up the bugs I'm aware of, so I think this is ready for an "is this good enough" kind of review (I don't like to jinx myself by saying "final" review 😄).
That's my main reasoning behind going with this PR for now — to give users the ability to adjust values here, where we can do more nuanced UI work in follow-ups for WP 6.6 🤞
Yes, it'd be great to consolidate controls for consistency. From my perspective, given the complexity of dealing with background images, I've been focusing on building out the missing features of the background image support. Then, once we've got parity in terms of features, we might look at how we could consolidate the two further down the track.
That's a good question — in the cases where the image is smaller than the container, we still want users to be able to set values that are percentage-based and not exactly top / left / right / bottom / center, so we'll likely still want a UI control for it. And by using the same focal point picker, we don't have a UI jump when adjusting between background sizes. Also, it's possible to have a fixed image that's larger than the container, so for now, I'm leaning toward going with the existing picker, and seeing if we can improve the UI for 6.6. For now, though, I've renamed the label from "Focal point" to "Position" so hopefully that makes it a little more general. Here's how it's looking now:
So, in terms of the discussion for whether or not this can make it in for 6.5, my main question is: Is re-using the existing focal point picker UI good enough for this feature for 6.5? If so, we can merge this in and have a bit more flexibility in the controls for this next release, knowing that the controls are hidden by default, so they'll only be exposed when folks reach for them. Alternately, if we're concerned about exposing the controls prematurely, we could skip the feature for this release and have another go at it with redesigned controls in 6.6. Personally, I think it's a pretty useful feature, so I'd like to see if we can get it in for 6.5, but this is very much a weakly held position, so very happy for any and all feedback about the idea 🙂 |
|
Flaky tests detected in d9d87ba. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7779288651
|
I like the congruency between the Cover block control background image controls - hopefully, as you say, they can be unified down the line. Another point on the the unit-based vs. matrix component: the subject of an image could be anywhere on the image canvas. I imagine therefore preset positions wouldn't be able to position all images in a way the user wants. For folks that are exporting theme.json from the site editor, this could be an important distinction. On another note: I assume we'd want to support more than just percentage units, e.g., |
Yes! That's a good point, too. Whatever the outcome of this PR, I'll write up a list of follow-ups to add to #54336 of all the extras we'd like to tackle. |
|
I don't have a problem with merging this and changing to a different control later. Nothing's ever permanent... I'll take this for another spin and look at the code again. |
|
One more thing: I don't mind it actually, but just flagging it as it's an edge case that we might have missed. You can add Size without having an image selected, which looks like this: 
|
|
Thanks @noisysocks!
It turns out the
Yeah, I think that's probably part of the awkwardness of using the ToolsPanel for this set of controls. For now, I think it's probably not too much of a problem as folks need to manually enable the controls in order to see them. But in future follow-ups, when we look at the popover idea, I think we'll likely guard the controls behind there being an active background image. I'll make a note of that for follow-ups. I think we're probably in a good place to land this PR now. If there are no objections, with the approving review from @noisysocks, I'll look to merge this PR in tomorrow 🙂. Let me know if anyone feels like any of the wrinkles uncovered so far are blockers to landing! |
|
👍 |
andrewserong
deleted the
try/add-background-position-controls-to-background-image-support
branch
andrewserong
changed the title
|
Thanks folks! I've added the suggested improvements so far to the tracking issue (#54336) under a separate heading for background position. Let me know if I've missed anything and I can update. |
What?
Part of: #54336
Try adding background position controls to the background image block support used by the Group block.
Why?
So that folks can choose a focal point / background position within the set background image. This offers a lot more flexiblity to using
ContainorFixedsizes.How?
Re-use the
FocalPointPickercomponent and map coords to a realbackgroundPositionCSS value.To-do
For now, I mostly wanted to gather feedback to see if this feels viable for 6.5. Before a final review, I'll need to tidy up:
Testing Instructions
Screenshots or screencast
2024-02-02.16.05.07.mp4