Update cover block min height control to use HeightControl #62509
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
t-hamano
reviewed
Jun 27, 2024
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.
Comment on lines
160
to
163
| 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();
Comment on lines
375
to
376
| const [ heightControl ] = | ||
| screen.getAllByLabelText( /Minimum height/ ); |
There was a problem hiding this comment.
Suggested change
| 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:
|
t-hamano
requested changes
Jul 8, 2024
|
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes: #56020
This PR when applied,
Replace
UnitControlcomponent withHeightControlin cover blockScreenshots or screencast