Block Support: Add height block support feature #32499
Closed
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
aaronrobertshaw
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
oandregal,
spacedmonkey and
TimothyBJacobs
as code owners
aaronrobertshaw
added
[Feature] Design Tools
aaronrobertshaw
changed the base branch from
trunk
to
try/block-supports-dimensions-panel
|
Size Change: +299 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
add/height-block-support
branch
from
65bcd99 to
20d74c6
Compare
8 tasks
|
Very nice. Love the progressive panel, and the height control doesn't suffer from the same challenge that the width control does. |
7 tasks
aaronrobertshaw
force-pushed
the
try/block-supports-dimensions-panel
branch
from
f140452 to
01cf433
Compare
aaronrobertshaw
force-pushed
the
try/block-supports-dimensions-panel
branch
from
4726898 to
2cf8fc6
Compare
aaronrobertshaw
force-pushed
the
try/block-supports-dimensions-panel
branch
from
b63b4e6 to
7c988d6
Compare
aaronrobertshaw
force-pushed
the
add/height-block-support
branch
from
063a7e9 to
d75884e
Compare
aaronrobertshaw
force-pushed
the
try/block-supports-dimensions-panel
branch
from
ea98a10 to
dbaa31c
Compare
aaronrobertshaw
force-pushed
the
add/height-block-support
branch
from
d75884e to
63e8027
Compare
This would be a great addition @aaronrobertshaw. I thinking specifically about the cover block mentioned in this issue: #23770 (comment) I could try to tackle adding it if you haven't already. It would mean adding two new hooks similar to what you've done in the PR I assume for now, in the absence of any designs consolidating width/min-width/max-width and so on...? |
I haven't started on min/max heights or widths yet. I was holding off until I got some more traction on the primary height/width block supports. It would be great if you'd like to pick those up. The sooner they are in place the smoother we can transition the Cover block's ad hoc min-height to use all block support features and unify its spacing and dimensions panels. |
7 tasks
aaronrobertshaw
force-pushed
the
add/height-block-support
branch
from
63e8027 to
6c89606
Compare
ramonjd
added a commit
that referenced
this pull request
Aug 11, 2021
oandregal
reviewed
Aug 11, 2021
| @@ -21,10 +21,10 @@ function gutenberg_register_dimensions_support( $block_type ) { | |||
| return; | |||
| } | |||
|
|
|||
| $has_spacing_support = gutenberg_block_has_support( $block_type, array( 'spacing' ), false ); | |||
| // Future block supports such as height & width will be added here. | |||
| $has_dimensions_support = gutenberg_block_has_support( $block_type, array( '__experimentalDimensions' ), false ); | |||
There was a problem hiding this comment.
Sorry for being late for the conversation. I've noticed that in #32392 we renamed a lot of things from spacing to dimensions. Couldn't look at it deeply yet, but wanted to flag that changing keys in block.json or theme.json is problematic if they already landed in WordPress core.
For example, here we're changing spacing to __experimentalDimensions in block.json. But spacing has already landed in WordPress core, so we need to keep spacing working as before. If we absolutely need to rename it, we can add some code that converts spacing into dimensions. If it's just a name preference, I'd advise to keep using what we have (spacing) and don't make our work harder for this.
At the UI level, it can be labeled differently if we need to and we also can change it as many times as we need.
There was a problem hiding this comment.
I've gone through a first pass at #32392 I may have found an issue https://github.com/WordPress/gutenberg/pull/32392/files?file-filters%5B%5D=.js&file-filters%5B%5D=.json&file-filters%5B%5D=.php#r686752790
If we keep the spacing key, I'd suggest we rename the things back to spacing internally, although at the UI level we name them "dimensions".
There was a problem hiding this comment.
Couldn't look at it deeply yet, but wanted to flag that changing keys in block.json or theme.json is problematic if they already landed in WordPress core.
Thanks @nosolosw, I'd thought of that and thought I'd maintained that backwards compatibility. Happy to change whatever I've messed up. I'm missing where that is though.
For example, here we're changing spacing to __experimentalDimensions in block.json. But spacing has already landed in WordPress core, so we need to keep spacing working as before.
I've actually not changed spacing to __experimentalDimensions in the block.json or theme.json. In the line below this comment you can still see the check for the spacing portion of the overall dimensions themed block support.
My plan was to introduce the new specific dimensions related supports under "dimensions" itself. It didn't make sense to me to have height, width etc under spacing.
Some points about where the separation of dimensions and spacing keys/support occurs are:
lib/class-wp-theme-json-gutenberg.phpaddsdimensionsin addition to, rather than replacingspacinglib/theme.jsonadds the setting fordimensions.customHeightseparate tospacingpackages/block-editor/src/hooks/dimensions.jschecks separate block.json flags forspacingvs__experimentalDimensionssupportpackages/block-editor/src/hooks/style.jsdimensions key was added alongside spacing for the inline style generation- The padding/margin styles are still under
spacingwithin the block attributes style object - The written tests also indicate that the spacing keys were still maintained.
Your comment regarding the key used when registering the block support in dimensions.php was well made and I have a fix for that specifically in #34030.
Is there anything else I have missed? Thanks for your help as always!
There was a problem hiding this comment.
Thanks for the quick turnaround, Aaron. I have a second thought about the other PR. I can review this one properly once we clarify whether what I've brought up is an issue or not.
There was a problem hiding this comment.
I've replied on the other PR seeking a little clarification on the desired changes. Hopefully, I'll have that fixed up tomorrow. Then will make the required changes here.
There was also some work based on this PR around adding min-height support and using it to unify two "dimensions" panels in the Cover block. It looks like that may also need some further discussion although the hope was to have that sorted before the next release.
Really appreciate your insight on the dimensions/spacing backwards compatibility issue with core. Thanks!
This was referenced Aug 30, 2021
ramonjd
added a commit
that referenced
this pull request
Aug 31, 2021
7 tasks
aaronrobertshaw
force-pushed
the
add/height-block-support
branch
from
1bd0bf3 to
5b17ec8
Compare
36 tasks
aaronrobertshaw
force-pushed
the
add/height-block-support
branch
2 times, most recently
from
7c3ac3f to
3c62628
Compare
7 tasks
aaronrobertshaw
force-pushed
the
add/height-block-support
branch
from
3c62628 to
840b77d
Compare
aaronrobertshaw
requested review from
ajlende,
mkaz and
ryanwelcher
as code owners
After the release of 5.9, the theme.json php class related changes will be added via a separate isolated commit.
Rename wp_get_global_settings to gutenberg_get_global_settings. In WordPress 5.9 the wp_* function is already defined, so we can't override them. It's calling the existing WP_Theme_JSON classes in WordPress core so it won't pick up the plugin modification. In the plugin, to make sure these changes work fine in 5.9 as well, we need to use the gutenberg_* function instead.
…e Gutenberg classes
While this PR does not do any behavioral change, we still need to call the plugin clasess when the minimum plugin version is 5.9 (at which point, the compat/5.9 is removed).
aaronrobertshaw
force-pushed
the
add/height-block-support
branch
2 times, most recently
from
f32466c to
757dc48
Compare
aaronrobertshaw
force-pushed
the
add/height-block-support
branch
from
757dc48 to
0023551
Compare
|
I've cherry-picked some commits from #38883 to allow the proper extension of theme.json and global styles. At this time, this PR is working however there has been a recent change of plans around how we might manage the general layout of blocks. This might actually include new layout blocks specifically. It is yet to be seen how appropriate control over height and width will be within this context. As a result, I will close this PR and the ones related to width support (#32502, #32878) |
1 task
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.
Related: #28356
Depends on: #32392
Description
Adds height block support and includes in new Dimensions panel.
How has this been tested?
Theme.json test:
wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose --filter stylesheet /var/www/html/wp-content/plugins/gutenberg/phpunit/class-wp-theme-json-test.php'Style.js test:
npm run test-unit:watch packages/block-editor/src/hooks/test/style.jsManual Test Instructions
By Block Typetab.Screenshots
Types of changes
New feature.
Next Steps
PR for min-height: Block Support: Add min-height block support #33970
Checklist:
*.native.jsfiles for terms that need renaming or removal).