Backport min-height block support feature #3940

andrewserong · 2023-01-30T04:25:13Z

This PR backports the minimum height block support feature's PHP changes from Gutenberg. Those changes were originally merged in the following PRs:

The opting-in of the block support for particular blocks, and UI support will land separately as part of the main JS package update for 6.2 — hopefully, this PHP change should be able to safely land in isolation.

Trac ticket: https://core.trac.wordpress.org/ticket/57582

Aside from the included tests, I'm not sure if there's a reasonable way to test this prior to the JS packages update landing, since that will also include the block support opt-ins for group and post-content blocks. However, it might be possible to manually update the block.json file for the post template block to include dimensions.minHeight under supports, and then add the following block markup to a template in the site editor (e.g. the single template):

<!-- wp:post-content {"style":{"dimensions":{"minHeight":"80vh"}},"layout":{"type":"constrained"}} /-->

On the site-frontend (assuming the site editor allows you to save the above block markup), the post content block should be rendered with minHeight set to 80vh. If this doesn't work in manual testing, however, then we might need to wait until the JS packages have been updated before testing manually.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

tests/phpunit/tests/theme/wpThemeJson.php

costdev

Thanks for the PR @andrewserong! I've left few thoughts below 🙂

tests/phpunit/tests/block-supports/dimensions.php

src/wp-includes/class-wp-theme-json.php

hellofromtonya · 2023-01-30T14:59:25Z

src/wp-includes/block-supports/dimensions.php

@@ -29,8 +29,7 @@ function wp_register_dimensions_support( $block_type ) {
 		return;
 	}

-	$has_dimensions_support = block_has_support( $block_type, array( '__experimentalDimensions' ), false );
-	// Future block supports such as height & width will be added here.
+	$has_dimensions_support = block_has_support( $block_type, array( 'dimensions' ), false );


Results from a wp.org search:

0 plugins found using __experimentalDimensions https://wpdirectory.net/search/01GR1JTT957N4BM1FAZAHPCQ1C

0 themes found using __experimentalDimensions https://wpdirectory.net/search/01GR1JX1E9HM3BQQQ0GJ457B68

So I'm wondering:

What are the backwards-compatibility risks for users who may have a custom plugin or theme using it?

How was '__experimentalDimensions' previously exposed and used?

What is the mitigation plan for '__experimentalDimensions' to upgrade blocks using this setting?

One way to mitigate:

Check it here with an ||.

Throw a deprecation notice to alert to use the new 'dimensions' instead.

Thanks for raising this one @hellofromtonya — this is a pretty interesting case for stabilisation in that the support technically existed prior to this PR (with the name __experimentalDimensions) however it was a no-op, in that it didn't actually expose any controls or styles. So even if a plugin had opted in to using it (which is pretty unlikely), there would have been no behaviour from it. Because min-height is a new feature, and the dimensions controls were never exposed or promoted anywhere, I don't think we need to add any handling to deal with the former __experimentalDimensions name — it appears it was mostly there as a placeholder in code for future features.

What do you think?

If it was a no-op, I think we're good to leave it as is.

Thank you @andrewserong and @ntsekouras for the explanation.

tl;dr
No BC concern.

Let me summarize the reasoning:

Yes, this __experimentalDimensions setting existed before 6.2.

It did nothing. No functionality was attached to it. (think of it as placeholder for the future, ie today)

If a developer is using it, there is no effect to them for removing it.

I agree then. It's not a BC concern to remove it. Thank you!

src/wp-includes/block-supports/dimensions.php

Co-authored-by: Mukesh Panchal <[email protected]>

tests/phpunit/tests/block-supports/dimensions.php

Co-authored-by: Colin Stewart <[email protected]>

ntsekouras

Thank you @andrewserong !

I have manually tested the front end by adding

<!-- wp:post-content {"style":{"dimensions":{"minHeight":"80vh"}},"layout":{"type":"constrained"}} /-->

and adding the support in the block, and works as expected in the front end - that means applying the minHeight styles.

hellofromtonya · 2023-02-01T14:00:13Z

Status: Currently reviewing and testing for commit consideration.

hellofromtonya · 2023-02-01T15:00:48Z

tests/phpunit/tests/block-supports/dimensions.php

@@ -0,0 +1,104 @@
+<?php


The file and the class need renaming for Core's Test coding standards and consistency.

File wpApplyDimensionsSupport.php

Test Class: Tests_Block_Supports_wpApplyDimensionsSupport

Will push a commit shortly.

Whoops forgot to share the naming convention:

For the file:
It's the function's name in camelCase without the _.

For the test:
It's in this format:

Tests_{group}_{functionNameInCamelCase}

See handbook here https://make.wordpress.org/core/handbook/testing/automated-testing/writing-phpunit-tests/#test-classes for more information.

Note: In Gutenberg, more work is needed in the linting before its tests can be converted into Core's coding standards.

@hellofromtonya According to the handbook, it says the test class should be in this format:

Tests_{Group}_{FunctionNameInTitleCase}

Thus, the class Tests_Comment_GetCommentClass(), which contains tests for the get_comment_class() function, is located in tests/comment/getCommentClass.php.

What I'm not sure about though, is whether {Group} should be BlockSupports, or Block_Supports, i.e.

Tests_BlockSupports_WpApplyDimensionsSupport OR

Tests_Block_Supports_WpApplyDimensionsSupport

@costdev The other test in the group has it as Block_Supports. So I felt it that way for consistency and because the handbook is unclear if the group should also be smooched together. Thinking that topic needs a Trac ticket of its own to discuss and get consensus.

Regarding _Wp rather than _wp, see this ticket with support for encouraging Wp for consistency among all test classes, despite there being more cases of _wp in the test suite historically. I'm happy to go with what you decide regarding the capitalization after viewing the ticket. 🙂

Oh wait I do see what you mean though about capitalizing the first letter of the function's name. Yup that needs changing. Good catch. Will do that in the commit.

Capitalization done here 12cebb4. Thanks @costdev!

hellofromtonya

Confirmed the PHP changes from the GB PRs are here ✅
Confirmed there's no BC issue for the removal of __experimentalDimensions setting ✅
Reviewed for coding standards ✅
Tests are okay ✅
@ntsekouras tested it ✅

LGTM 👍 Will prepare the commit once the CI jobs finish and are 🟢

hellofromtonya · 2023-02-01T16:14:57Z

Committed via https://core.trac.wordpress.org/changeset/55175.

andrewserong · 2023-02-01T21:59:23Z

Wonderful, thank you for landing this one, and for the extra discussion, I learn more with each of these PRs 🙂

andrewserong added 2 commits January 30, 2023 15:22

Backport min-height block support feature

a668ff3

Fix failing tests

c2bb6cd

mukeshpanchal27 reviewed Jan 30, 2023

View reviewed changes

tests/phpunit/tests/theme/wpThemeJson.php Outdated Show resolved Hide resolved

Fix linting issue

c3be880

andrewserong marked this pull request as ready for review January 30, 2023 04:43

andrewserong mentioned this pull request Jan 30, 2023

Plugin: Backport PHP changes for WordPress 6.2 release WordPress/gutenberg#47187

Closed

85 tasks

Add missing line

df4d498

costdev requested changes Jan 30, 2023

View reviewed changes

tests/phpunit/tests/block-supports/dimensions.php Outdated Show resolved Hide resolved

tests/phpunit/tests/block-supports/dimensions.php Outdated Show resolved Hide resolved

mukeshpanchal27 reviewed Jan 30, 2023

View reviewed changes

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved

ntsekouras requested a review from hellofromtonya January 30, 2023 12:26

hellofromtonya reviewed Jan 30, 2023

View reviewed changes

src/wp-includes/block-supports/dimensions.php Show resolved Hide resolved

andrewserong and others added 4 commits January 31, 2023 15:41

Add full stop to comment

e79718f

Co-authored-by: Mukesh Panchal <[email protected]>

Simplify tests

6b1b594

Fix linting issue

19fa05e

Empty commit to rerun tests

432be91

costdev reviewed Feb 1, 2023

View reviewed changes

tests/phpunit/tests/block-supports/dimensions.php Outdated Show resolved Hide resolved

Update data provider docblock.

69e218f

Co-authored-by: Colin Stewart <[email protected]>

ntsekouras approved these changes Feb 1, 2023

View reviewed changes

hellofromtonya reviewed Feb 1, 2023

View reviewed changes

hellofromtonya added 3 commits February 1, 2023 09:02

Renames test file for coding standard

adb55ca

Rename test class to coding standards

cf79639

Minor formatting in tests

811495e

hellofromtonya approved these changes Feb 1, 2023

View reviewed changes

Capitalize 1st letter of function name

12cebb4

hellofromtonya closed this Feb 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport min-height block support feature #3940

Backport min-height block support feature #3940

andrewserong commented Jan 30, 2023 •

edited

Loading

costdev left a comment

hellofromtonya Jan 30, 2023

hellofromtonya Jan 30, 2023

andrewserong Jan 30, 2023 •

edited

Loading

ntsekouras Jan 31, 2023

hellofromtonya Jan 31, 2023

ntsekouras left a comment

hellofromtonya commented Feb 1, 2023

hellofromtonya Feb 1, 2023

hellofromtonya Feb 1, 2023

costdev Feb 1, 2023 •

edited

Loading

hellofromtonya Feb 1, 2023

costdev Feb 1, 2023 •

edited

Loading

hellofromtonya Feb 1, 2023

hellofromtonya Feb 1, 2023

hellofromtonya left a comment

hellofromtonya commented Feb 1, 2023

andrewserong commented Feb 1, 2023

Backport min-height block support feature #3940

Backport min-height block support feature #3940

Conversation

andrewserong commented Jan 30, 2023 • edited Loading

costdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewserong Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntsekouras left a comment

Choose a reason for hiding this comment

hellofromtonya commented Feb 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costdev Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costdev Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellofromtonya left a comment

Choose a reason for hiding this comment

hellofromtonya commented Feb 1, 2023

andrewserong commented Feb 1, 2023

andrewserong commented Jan 30, 2023 •

edited

Loading

andrewserong Jan 30, 2023 •

edited

Loading

costdev Feb 1, 2023 •

edited

Loading

costdev Feb 1, 2023 •

edited

Loading