Cover block: clear the min height field when aspect ratio is set #59191
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
andrewserong
added
[Type] Bug
|
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. |
andrewserong
changed the title
andrewserong
added
the
Backport to WP 6.7 Beta/RC
9 tasks
|
Size Change: +5 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a9e2847. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7968765344
|
aaronrobertshaw
approved these changes
Feb 21, 2024
There was a problem hiding this comment.
Nice tweak @andrewserong 👍
This tests as advertised for me.
Initially, I thought it was a little weird not resetting the min-height attribute value as I thought the block support did that. Then I realised this uses its own ad hoc min-height solution.
Is this so the min-height can be reapplied when clearing an aspect ratio, or is it just because the Cover block supported min-height before the block support was available and was never switched over?
Do you think there's any plan/desire to switch the Cover block to use the block support?
| Before | After |
|---|---|
Screen.Recording.2024-02-21.at.11.42.38.am.mp4 |
Screen.Recording.2024-02-21.at.11.43.44.am.mp4 |
|
Thanks for the review @aaronrobertshaw! 🙇
It's primarily because the Cover block supported min-height before the block support was available, and this PR was the simplest way to fix the issue for 6.5, without having to add a But yes, I think ideally the Cover block would use the min height block support, in which case we wouldn't need this ad hoc fix for the ad hoc control. Perhaps something to consider for 6.6 could be to explore using the min height block support — I think the main thing that could make it a worthwhile effort would be that then folks could set a min height for the cover block in global styles. |
andrewserong
deleted the
fix/cover-block-empty-min-height-field-when-aspect-ratio-is-set
branch
|
I just cherry-picked this PR to the cherry-pick-wp-6-5-beta-3 branch to get it included in the next release: 1fa7012 |
getdave
added
Backported to WP Core
9 tasks
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?
Part of #58230 and follow-up to #56897
In the Cover block, when the aspect ratio is updated, clear out the value for the minimum height field.
Why?
When an aspect ratio is set, it will always override the minimum height value. To make it clearer that this is the case, display the minimum height field as empty. We don't need to "really" clear out the value itself in block attributes, as the minimum height value gets
unsetby the aspect ratio rules.How?
In the Cover block, pass in an empty string to the Cover block's minimum height input field if an aspect ratio value is set.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Before
2024-02-20.16.05.54.mp4
After
2024-02-20.15.59.56.mp4