Update cover block min height control to use HeightControl #62509
Conversation
|
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. |
akasunil
added
the
[Block] Cover
|
Size Change: -13.5 kB (-0.76%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
@richtabor Should we replace height control label "Minimum height of cover" with "Minimum height"? |
Yes, thanks! |
|
Hi @richtabor, Could you please review this PR? |
|
Seems good to me. I'm curious about |
This is the same component used in the dimensions panel (source). |
|
|
It was more a meta comment; I could see that component (unit control + range) being used for other numerical settings. E.g. width, margin, padding, etc. In fact Padding already uses a custom version of this pattern: 
It would be good to tidy that up (not in this PR, obviously) :) |
|
We could rename it to something like |
Maybe, let's consider that for a follow-up though. |
There was a problem hiding this comment.
Thanks for the PR! Overall it looks good.
We might be able to migrate to the block support (supports.dimensions.minHeight) in the future, but we'd need to add Block deprecation, which would be a big change, so for now I think this approach is fine.
| const defaultHeightValue = await coverBlockEditorSettings | ||
| .getByLabel( 'Minimum height of cover' ) | ||
| .getByRole( 'slider', { name: 'Minimum height' } ) | ||
| .inputValue(); | ||
| expect( defaultHeightValue ).toBeFalsy(); | ||
| expect( defaultHeightValue ).toEqual( '0' ); |
There was a problem hiding this comment.
This test is trying to verify that the minimum height is "undefined", but it changes the expected result.
I think the actual value being checked is a field for entering a number, not a slider.
Can't the following code achieve the same thing without changing the expected result?
const defaultHeightValue = await coverBlockEditorSettings
.getByRole( 'spinbutton', { name: 'Minimum height' } )
.inputValue();
expect( defaultHeightValue ).toBeFalsy();| const [ heightControl ] = | ||
| screen.getAllByLabelText( /Minimum height/ ); |
There was a problem hiding this comment.
| const [ heightControl ] = | |
| screen.getAllByLabelText( /Minimum height/ ); | |
| const heightControl = screen.getByRole( 'spinbutton', { | |
| name: 'Minimum height', | |
| } ); |
It is better to use accessible selectors.
| unprocessedValue !== '' | ||
| ? parseFloat( unprocessedValue ) | ||
| : undefined; | ||
| // eslint-disable-next-line @wordpress/no-unused-vars-before-return |
There was a problem hiding this comment.
I'd like to avoid disabling ESLint errors if possible. Can I use logic like the following to deal with this?
const parsedQuantityAndUnit =
parseQuantityAndUnitFromRawValue( unprocessedValue );
const currentValue = parsedQuantityAndUnit[ 0 ];
if ( isNaN( currentValue ) && currentValue !== undefined ) {
return;
}
const currentUnit = parsedQuantityAndUnit[ 1 ];
onUnitChange( currentUnit );
onChange( currentValue );This is similar to this part.
|
Sorry for the late response. I thought we could ship this PR, but there was one thing that bothered me. That is, when I move the slider and it reaches zero, the height of the Cover block suddenly becomes higher. a6f72cbd2eda3aef107c8ccaeda92cde.mp4This is based on the following two facts:
In other words, the moment the slider reaches zero, To avoid this, we need to remove Changing this means that the output markup will change, so we need to add Block Deprecation. If we are going to add a block deprecation anyway, I think it might be better to migrate to block support now. Specifically, the following measures are required:
// From...
<!-- wp:cover {"minHeight":0,"minHeightUnit":"px","layout":{"type":"constrained"}} -->
<div class="wp-block-cover">
<span aria-hidden="true" class="wp-block-cover__background has-contrast-2-background-color has-background-dim-100 has-background-dim"></span>
<div class="wp-block-cover__inner-container"></div>
</div>
<!-- /wp:cover -->
// To...
<!-- wp:cover {"style":{"dimensions":{"minHeight":"0px"}},"layout":{"type":"constrained"}} -->
<div class="wp-block-cover" style="min-height:0px">
<span aria-hidden="true" class="wp-block-cover__background has-contrast-2-background-color has-background-dim-100 has-background-dim"></span>
<div class="wp-block-cover__inner-container"></div>
</div>
<!-- /wp:cover -->
Would you like to try this approach? |
Sorry, I found this suggestion inappropriate.
After all, I think it's difficult to migrate to block support at this point. Therefore, the additional approach I propose is as follows. Sorry for the confusion 🙇♂️
const minHeight =
minHeightProp !== undefined && minHeightUnit
? `${ minHeightProp }${ minHeightUnit }`
: undefined;
const style = {
minHeight,
};
<!-- From... -->
<!-- wp:cover {"minHeight":0,"minHeightUnit":"px","layout":{"type":"constrained"}} -->
<div class="wp-block-cover">
<span aria-hidden="true" class="wp-block-cover__background has-background-dim-100 has-background-dim"></span>
<div class="wp-block-cover__inner-container">
<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->
</div>
</div>
<!-- /wp:cover -->
<!-- To... -->
<!-- wp:cover {"minHeight":0,"minHeightUnit":"px","layout":{"type":"constrained"}} -->
<div class="wp-block-cover" style="min-height:0px">
<span aria-hidden="true" class="wp-block-cover__background has-background-dim-100 has-background-dim"></span>
<div class="wp-block-cover__inner-container">
<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->
</div>
</div>
<!-- /wp:cover -->
|
|
@aaronrobertshaw @andrewserong As we move forward with this PR, I'd appreciate some feedback on whether this approach in this comment makes sense 🙇♂️ The history of this PR can be summarized as follows:
|
|
Why not just set the minimum value of the RangeControl to 50, which is what it is effectively in trunk? That would be 1:1 to supporting today's experience with the new component, right? If you try to set |
We could do that, what do you say @t-hamano? |
This approach might also be possible. However, the |
|
Is there any way forward on this effort, or should we close this? |
@t-hamano and @akasunil are probably best placed to make the call however my read of the discussion so far is that this PR just needs updating to the settled on approach (including the 50px min tweak?).
That component is only in the block-editor package so might have more freedom to be updated. I may be misremembering but at one stage I thought we had some controls that allowed minimum values defined per unit e.g. px vs em etc. A similar thing could be accepted in that new prop.
This might not be a solution but the theme.json data is filterable. The min height block support could also be enabled by default for the Cover block type within the default theme.json file. So by that I mean, it stays as That might be worth exploring if we want to consolidate around block supports and limit deprecations. |
That was the In trunk:
in this PR:
Difference here i could find is only min value for
This is something can be explored but, it would required PHP change for filter which required back-porting, we will also need deprecation for this too. |
What?
Fixes: #56020
This PR when applied,
Replace
UnitControlcomponent withHeightControlin cover blockScreenshots or screencast