Cover block: Allow setting the height from the placeholder state. #35068
Conversation
ramonjd
added
[Status] In Progress
|
Size Change: +237 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
try/cover-min-height-placeholder-resizer
branch
from
5706513
to
f8ed363
Compare
ramonjd
changed the title
There was a problem hiding this comment.
Nice work @ramonjd! This is testing well for me, and it's great being able to interact with the minimum height while in the placeholder state. I've left a comment about whether it'd be worth constraining the minimum height in the placeholder state to leave clearance for the placeholder, and a tiny comment about the classnames (but feel free to ignore if it isn't relevant).
The wording looks good to me, but CC: @apeatling since he's been working on the placeholder wording, too.
| setAttributes( { minHeight: newMinHeight } ); | ||
| setTemporaryMinHeight( null ); | ||
| } } | ||
| showHandle={ isSelected } |
There was a problem hiding this comment.
I was wondering if it'd be worth adding the minHeight={ 250 } prop to this ResizableCover so that it gets passed down to ResizableBox? While values less than 250 are valid for the cover block, it looked a little odd being able to drag the control over the placeholder area. This shouldn't affect the ability to have a smaller height once a background colour is chosen 🤞
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
Good idea. I'll have a play around with your suggestion.
| .wp-block-cover.is-placeholder .cover-block__cover-placeholder-container { | ||
| background: $white; | ||
| width: 100%; | ||
| .block-editor-media-placeholder { |
There was a problem hiding this comment.
Tiniest of nits: it looks like the box-shadow comes from the .components-placeholder class, but the element here also happens to have the .block-editor-media-placeholder class name. Would it be worth switching to the other class name for consistency, or is it better to keep it as-is for specificity?
There was a problem hiding this comment.
This will need massaging as well, as we need the resting and focus states to be the same as other placeholders, which very probably requires we use a box shadow after all. Happy to help if this turns gnarly.
There was a problem hiding this comment.
Thanks for noticing this! I'll work on it and get back to you if the gnarliness defeats me! :)
|
Nice, this is working better than I had expected. I was happy to see the min height also being present in the inspector. Here's a GIF showing the cover resize: A few small things:
Some of the larger stuff, Andrew suggests adding a min-height while it's still in a placeholder state. That might be a good temporary solution, but it doesn't feel like a longer term solution to have separate min-heights depending on state. One thing that might or might not work would be to enhance the resizeobserver system that's built into the |
|
Thanks @andrewserong @apeatling and @jasmussen for the feedback. I didn't catch the min-height of the container and the focus states, so it's really helpful. I'll work on your suggestions and aim to get a revision up soon. |
ramonjd
force-pushed
the
try/cover-min-height-placeholder-resizer
branch
from
f8ed363
to
1ff0d25
Compare
|
I've converted this PR back to draft in order to experiment. Firstly, sorry for the wall of text and images. I thought it best to document my progress and gather feedback here. Secondly, it turns out most of the style/outline/focus issues can be resolved by moving the ResizableCover component inside the CoverPlaceholder component. 🎉
Actually, having played around with things I agree. For example, giving the resizer a static min-height of This is so the initial state of both matches up and the resize handler knows where to position itself, and there's no overlap. Here's what happens when we don't do this and insert a Cover Block:
Furthermore, a value of So for example, we might set To demonstrate what I'm talking about, here's what happens when we set the min-height to
Thanks for suggesting this. 🙇 So observing the height as well as the width in the Placeholder component is trivial using the const [ resizeListener, { width, height } ] = useResizeObserver();
let modifierClassNames;
if ( typeof width === 'number' ) {
modifierClassNames = {
// TODO rename these classes, e.g., `is-width-large`, `is-height-large` etc
'is-large': width >= 480,
'is-medium': width >= 160 && width < 480,
'is-small': width < 160,
// TODO determine the most appropriate breakpoint values for height
'is-tall': height >= 250,
'is-middle': height >= 100 && height < 250,
'is-short': height < 100,
};
}– it's how we apply it to this specific use case. Using a combination of the vertical and horizontal classes we might pull off a consistent experience of the following:
It's a promising direction. The placeholders might be taller or shorter depending on the content however, so it's been challenging to reconcile where and when we hide and show elements. Before I dive down that rabbit hole, it would be great to know if I've understood your comments @jasmussen 😆 Another option (the one I've just committed), is one that allows us to test the behaviour with a smaller footprint, would be to limit the resize behaviour to .components-resizable-box__container {
display: none;
}
.components-placeholder {
&.is-large {
min-height: 250px;
.components-resizable-box__container {
min-height: 250px;
display: block;
}
}Here's what that would look like:
The benefit would be that we expose the functionality, and then work in parallel on exposing vertical Placeholder classes. |
ramonjd
force-pushed
the
try/cover-min-height-placeholder-resizer
branch
from
1ff0d25
to
20ddf00
Compare
I like the sound of that. Instead of creating separate cases for width and height, though, I think it might be worth it to just lump them together as one, to keep things simple. So instead of: maybe something like: On the one hand, many third party blocks might be leveraging the existing classes, so it's risky to rename them. Additionally, by simply having at most three sizes to handle in the placeholder, it makes it easier to handle the responsive behavior across all placeholders, which is important if we are to keep using it. I also think, separate from this PR, that we can enhance the design of these placeholder components across the board, and in doing so find a better design for the minimal version of placeholders. For example, here's what a minimal site logo placeholder could look like, and here's a work in progress exploration on adding the color control to the Cover block toolbar, which would at least make it available in the smaller breakpoints. |
Definitely. What's more, I noticed that Cover Blocks within narrow columns for example, can be both very tall but narrow, so it'd be a difficult one to categorize. It might be worth an experimental PR just to see what's possible with various placeholder dimensions in light of the work you've linked to (thank you for that too!). |
There's definitely work to do, including designwise. But in light of that, it's also good to keep things as simple as possible, as simplicity is definitely what we'll seek more of for any iterations! Nice work. |
ramonjd
force-pushed
the
try/cover-min-height-placeholder-resizer
branch
from
20ddf00
to
4bb5315
Compare
|
More testing reveals that the resizer is sitting on top of the controls, meaning that we can't interact with the upload/media library buttons.
I suspect the reordering of the resizeable placeholder component in e97b6937536b4087248db7717f73c1b2e0edd987 Investigating... |
ramonjd
force-pushed
the
try/cover-min-height-placeholder-resizer
branch
from
4bb5315
to
7145dff
Compare
|
@apeatling Thanks for the review! I've reordered the markup and added a z-index to the placeholder so that we can interact with the controls this time 😄 Everything should work as before if you get time to do another round of reviewing. 🙇 |
|
Visually this is looking good, and it's a nice feature. However I'm seeing a 250px min-height on the placeholder: One problem that exists with enforcing a min-height just to accommodate the big placeholder, is that it doesn't account for the 250px is also taller than the 220 ish px that seem needed in my testing. It seems like we could possibly land this by reducing that min-height. However I think it's worth doing or ticketing a followup to revisit the cover placeholder and enhance its narrow-context behavior. It seems like with nice flex rules, we should be able to reduce the required min-height all the way down to about 72px:
Bad inspector mockup, we'd of course want things to align. |
True. That's why we restricted the resizer to show only for
WIll do! Thanks! 🙇
For sure. I think we should be able to find a way to have the placeholder content respond to the container's dimensions. I really only spent an hour or two hacking at it. How that plays with the resizer, assuming we unleash it across medium and small dimensions is well is to be seen 😄 |
The copy is new so it's still untranslated, but I think the 250px was originally set to accommodate two lines of text, which might be required for German or Russian for example. At 220px we get a bit of overlap:
It's not a huge eyesore I think. 240px seems to be the spot where we don't see this effect if we want to avoid it. It does expose fragility in that we're relying on unknown content height. We should try to overcome this limitation in future explorations. 👍 Cheers! |
It seems fine to hide elements, including the description and color swatches, as the placeholder gets resized, same as we already do for horizontal narrowing. So long as we come back to this, probably just pick a value and go! |
…lor (white) and border.
…etermine the min-height of the resizeable placeholder.
Reverting changes to the Placeholder `useResizeObserver` since it was a test. limit the resize behaviour to `is-large` placeholders
…he containing block
- a user can interact with the placeholder controls - we can target the resizer as a sibling of `.components-placeholder.is-large` Also cleaning up the CSS and removing unused styles.
ramonjd
force-pushed
the
try/cover-min-height-placeholder-resizer
branch
from
7145dff
to
eb6cfe3
Compare
… container so that our changes in editor.scss take precedence, we have to reinstate the COVER_MIN_HEIGHT of 50px now via css
There was a problem hiding this comment.
Nice work @ramonjd! This is testing nicely for me, and aside from the additional control while selected, the placeholder state looks consistent with other placeholders like the Image block. Like Joen, I noticed that the min height of the placeholder is now a little taller, however that also seems to help things in smaller viewport widths where the descriptive text wraps on a second line, so I think the extra breathing room there seems worth it (as you mentioned with factoring in the potential length of translations, too).
✅ Min height control in placeholder state updates values correctly
✅ Control is not shown on smaller Cover placeholder state when inserted within a Column block
✅ Placeholder's background and border colors are as they were before
✅ Interacting within the Cover block is working as before (selecting color, image, dragging an image from the desktop onto the placeholder, etc)
✅ Tested on Safari, FF, Chrome, and Edge on Mac and Safari and Chrome on iOS (iPhone 8)
LGTM! 🎉
|
Thanks again the thorough review @andrewserong 🙇 |
Description
This PR allows us to set the min-height value for the Cover Block by resizing the placeholder. 🪄
It's part of a wider issue to improve the Cover Block UX.
This PR also updates the copy in the placeholder to describe this functionality. Note, there's a PR to update the Cover Block placeholder copy, so we don't have to do it here.
kitty-resize.mp4
Kudos to @jasmussen for the original UX idea. 🎉
Resolves #34706
Testing
< 480pxTypes of changes
UX enhancement on a block placeholder.
Checklist:
*.native.jsfiles for terms that need renaming or removal).