Cover: Migrate the minimum height to the dimensions block support - #83447

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>
Co-authored-by: richtabor <richtabor@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

🤖 PR meta 🤖

📦 Bundle size

Size Change: -228 B (0%)

Total Size: 8.24 MB

_{734ec9e Run}

⚡ Performance

_{734ec9e Run}

Thanks for getting this PR up @t-hamano 🚀

One aside before the review proper. In other recent design tools work, I was seeing that we might need a closer look at how dimension presets interact with the core defaults in lib/theme.json. I suspect they don't de-dupe. FWIW I have some local explorations, but I need a window to shift focus onto it.

Backward compatibility holds up for the most part. Two blockers.

Blockers

The resize handle isn't gated with the rest. In your test 3 the control and the Full height button both go, but the drag handle stays. The user can still drag in a value they can't then see or clear, and it renders on the front end.

Nothing replaces the 50px floor. The shared DimensionControl hardcodes a floor of 0 and a block can't raise it. With presets defined, None writes 0 and takes the block below the 430px default.

That might well be what we want, clearing Cover's minimum height has been asked for often enough. I'd rather it was a decision than a side effect though.

Worth fixing

Two knock-ons from the opt-in. Core's theme.json needs the same entry landed with the backport. And anywhere theme.json settings aren't loaded, say a block editor embedded outside WordPress, the control goes entirely.

Block settings beat root settings, whatever the source. A theme turning dimensions.minHeight off at the root still gets the control on Cover, and has to opt out per block. Worth a line in the description.

Deprecation gaps. v1 to v3 declare minHeight and didn't get the migration, though nothing visible is lost there. No fixture covers v10 to v14 markup carrying minHeight either.

The attribute removal needs a dev note. Saved content is fine, anything reading minHeight or minHeightUnit isn't. The CHANGELOG entry also sits under Enhancements.

@aaronrobertshaw Thanks for the review!

The resize handle isn't gated with the rest. In your test 3 the control and the Full height button both go, but the drag handle stays. The user can still drag in a value they can't then see or clear, and it renders on the front end.

I disabled the UI for resizing when minHeight is disabled.

Nothing replaces the 50px floor. The shared DimensionControl hardcodes a floor of 0 and a block can't raise it. With presets defined, None writes 0 and takes the block below the 430px default.

That might well be what we want, clearing Cover's minimum height has been asked for often enough. I'd rather it was a decision than a side effect though.

My take is that allowing values below 50px, all the way down to 0, is acceptable. As for the None label, I think we might be abled to enhance the DimensionControl side—specifically, by having None apply undefined instead of 0, or by simply using a 0 label instead of None.

And anywhere theme.json settings aren't loaded, say a block editor embedded outside WordPress, the control goes entirely.

This applies to any effort to migrate attributes to block supports, doesn't it? I don't think it's an issue, as consumers can decide for themselves which npm version to use outside of WordPress.

Block settings beat root settings, whatever the source. A theme turning dimensions.minHeight off at the root still gets the control on Cover, and has to opt out per block. Worth a line in the description.
The attribute removal needs a dev note. Saved content is fine, anything reading minHeight or minHeightUnit isn't. The CHANGELOG entry also sits under Enhancements.

Once this PR is merged, I would like to summarize the key points for consumers in a dev note.

v1 to v3 declare minHeight and didn't get the migration, though nothing visible is lost there.

I think v1 to v3 don't declare minHeight

No fixture covers v10 to v14 markup carrying minHeight either.

Addressed in 734ec9e