Background image: move controls into a popover #60151
Conversation
ramonjd
added
[Type] Enhancement
|
Size Change: +723 B (+0.04%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
add/background-global-styles-panel
branch
from
b09f29e
to
653d32a
Compare
ramonjd
force-pushed
the
try/background-image-control-refactor
branch
from
77235c5
to
4e1c637
Compare
|
Notes: There's a bit of a blocker on this particular design since I'm not sure I can integrate tools panel functionality (show by default / reset) with the popover contents. The tools panel registers rendered tools panel items. Given that the background controls are in the dropdown popover, the items aren't rendered and so the tools panel ellipsis menus don't appear. I've tried adding a "dummy" |
ramonjd
force-pushed
the
try/background-image-control-refactor
branch
2 times, most recently
from
142aa5d
to
37f1319
Compare
ramonjd
force-pushed
the
try/background-image-control-refactor
branch
from
bdab991
to
0150ded
Compare
| focusable={ false } | ||
| className="block-editor-global-styles-background-panel__tab-panel" | ||
| > | ||
| <FocalPointPicker |
There was a problem hiding this comment.
One thing to note about the focal point picker is that its dimensions vary - it can be very tall or short depending on the image. This is whether it's in a popover or not, but it affect the popover height and scroll.
A tall image:
2024-06-20.15.26.45.mp4
Constraining it via max-height doesn't make the tool any more usable:
|
Nice work. Took a stab at some designs in this Figma. The main thing I wanted to address was to illustrate the idea of just moving the entire background image panel inside the flyout. That might also address the split behavior of this one, which doesn't feel as ergonomic as it could: Here are the results, but note that they should be thought of as in a draft state. @jameskoster shared some ideas that went in a different direction on another of these issues, and I'd like him to have a chance to chime in, and perhaps remix the figma pieces. Group block with no background image: 
Note how even the "Add background image" button was moved inside the flyout. Group block with a background image: 
The flyout essentially contain the contents of the panel, including tiling and position options. I didn't see a clear need for tabs yet, but as noted, Jay had some thoughts too that I wanted to give him a chance to illustrate. Additional note, this mockup tries a different design for the background image "swatch"—instead of being a circle, it is here, a square, to better match what we are showing in the List View for images, etc. |
This feels a bit of a drawback since it's adding a click. When there's no image set, could the button open the media library directly? There are options to upload or select an image there.
This mockup seems like a good direction for now. We're just missing controls to replace or unset the background image. For unsetting there are a couple of existing patterns to consider:
Having a 'Clear' button in the popover might be strange to use, because clicking it would hide all the other settings. For that reason I prefer the latter. It also makes unsetting require one less click. Replacing is slightly tricker. It's probably okay to place this affordance in the popover, perhaps as an 
|
|
The minus to unset is a great argument for making this follow the ItemGroup pattern. And feel free to see if we can keep the "Add background image" CTA right in the Item itself. But we need some way to then "replace" from inside the flyout, it probably shouldn't be that recycle button. |
|
Thanks for the continued support on this @jasmussen and @jameskoster Also for the Figma link, it helps a lot!
This is very doable. So,
I'll try it out in the next iteration. As mentioned over at #60100 (comment) it'd be nice to get the flyout/refactor done first and then work on relocating the background controls, e.g., in Color and Fill. These PRs get very chunky very quickly 😄 |
|
Took a stab at an i3 that incorporates the conversation of the reset button being added, and the initial state simply opening the media library. It's not quite working so well: 
I still think these are the right ingredients, with the majority of work happening in the modal. But there's something here that isn't connecting, and it's all related to @jameskoster's excellent observations here. The short is, I still think we probably want this panel to live in a flyout. But probably it's good to establish first: where does the button live? Jay, let me know if this inspires any ideas. |
|
In the short/medium term I don't mind putting the button in the Color (& Fill) panel. I also wouldn't mind leaving the Background Image panel and placing it there. I think both provide a pathway to a dedicated 'Background' or 'Fill' panel in the future, once we've fully explored that idea. |
ramonjd
force-pushed
the
try/background-image-control-refactor
branch
from
0150ded
to
822c82c
Compare
|
Testing out moving the media replace flow to the flyout panel: Kapture.2024-06-25.at.12.41.44.mp4A couple of janky issues, mainly the popover has a z-index of 
My instinct tells me the media library should always sit on top of everything, so bumping its z-index could work. The tricker path is closing the background image controls panel when the media library is open. |
|
Looks promising, though!
It seems we're doing that for a bunch of other modals, so sounds like it could be a good approach? gutenberg/packages/base-styles/_z-index.scss Lines 124 to 135 in 044a05f |
ramonjd
force-pushed
the
try/background-image-control-refactor
branch
from
f27323c
to
cab0f46
Compare
ramonjd
force-pushed
the
try/background-image-control-refactor
branch
from
94186e1
to
6495425
Compare
|
The squishening!
|
There was a problem hiding this comment.
Great work @ramonjd, and thanks for the guidance @jameskoster!
This is testing very nicely for me now:
✅ The position of the popover feels stable when toggling between sizes, thanks to the disabled fields
✅ The focal point position area no longer takes up very much room, making the controls feel more contained
✅ On smaller screen sizes (like my laptop screen) the popover feels reasonably sized and doesn't display a scrollbar
2024-07-02.15.12.25.mp4
Overall this very much crosses the threshold for me of feeling much nicer to use than what we have on trunk. In terms of the fixed height of the focal point picker I wonder if we could afford to give it a little more room potentially, but I reckon at this stage it'd be best to leave further tweaking to follow-ups since this is a pretty decent sized change in its current form.
Might be worth seeing what the designers think before landing, but overall this LGTM! Just left a tiny nit of a code comment that can be removed before merge.
🚀
andrewserong
added
the
[Feature] Design Tools
I had the same thought. As a small tweak, could the height scale based on viewport height? It would be really great if we could use the same unit control as you can see in the Padding setting here: 
The 'Size' segmented control should also use the new 40px sizing ( With these changes I think we're good to go :) |
I'll revert back, but note that removing
I did try Regardless, I think we're in a good place to iterate. I'll fix the above issues and merge. Thanks again for all the help! |
Refactor the media toggle button and refactor controls Temp. don't send default controls in hook
Icon margin tweak
Reset image functions reset all background image properties. isShownByDefault prop not used. Deleting Prevent focal point picker creating horizontal scroll Expand popover on mobile Don't resize with window height change
Squishing all the controls, including giving the focal point picker a max-height
Ensure long file names are not cut off by min-width Use clamp value for focal picker image height
ramonjd
force-pushed
the
try/background-image-control-refactor
branch
from
b25d3db
to
34d20ce
Compare
|
Okay! I think I've come up with a good compromise by using a clamp value for the focal picker image height. It can be tweaked later if needed. 
|
Nice one! 🚀 |
Just wanted to comment on this, the default size is 36px, and should ideally be avoided (see #46734) in favor of a taller and more legible 40px default, or exactly for space-saving concerns the more |
* Testing other component layout and dummy toolspanel items Refactor the media toggle button and refactor controls Temp. don't send default controls in hook * Rejigging components, catering for no-background-image-controls * With tabs A bit of margin * Revert tabs and move the image upload to the floating panel * Temp hack to push popover into the background * Fix hack to push popover into the background * Tweak size of media flow dropdown Icon margin tweak * Remove default controls override as there's only 'background image' now. Reset image functions reset all background image properties. isShownByDefault prop not used. Deleting Prevent focal point picker creating horizontal scroll Expand popover on mobile Don't resize with window height change * Rolling back expanding popover in mobile. Squishing all the controls, including giving the focal point picker a max-height * Remove commented-out code * Revert input sizes Ensure long file names are not cut off by min-width Use clamp value for focal picker image height Unlinked contributors: brentjett. Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
What?
This PR:
See:
Doing this with a view to consolidate colors and fill. See: #60100 (comment)
Also take into account:
TODO
Wire in tools panel reset for individual properties? E.g., position and repeat?Why?
To win back space in the sidebar controls.
How?
Moving background image controls into a flyout popover. This includes, where an image exists, the media select control.
To get the Tools Panel controls to work, I've created an invisible
ToolsPanelItemcomponents for each control set - this registers reset callbacks and so on in the Tools Panel.Testing Instructions
The flyout appears in the Global styles (Layout > Background Image) and at the block level for Group, Quote, Post Content blocks. For both:
Testing Instructions for Keyboard
Check:
Screenshots or screencast
Resetting background properties:
2024-06-28.10.20.00.mp4
Related