-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dimensions Panel: Add new ToolsPanel component and update spacing supports #32392
Changes from all commits
795fe99
dfb1403
0cb9f82
1b23fad
2487805
696070f
03c87c4
5fa856f
7fb7e8a
62d7895
5f7b39f
db0c413
dd015b0
659a79e
213d250
a96dc35
481ea39
0d75ba7
ffedb61
ae4a3b2
e7a89ea
07420bc
9ec400f
a4d96b0
17bd1b6
4e33e6c
3609b5a
5746e25
c8d96ee
7c22312
dbaa31c
8f196bc
76a2b8d
224c0ea
8343c9b
6068bec
48388f5
92bcf52
a624202
eaad38e
8a7df11
ccee3ac
f2851b6
3d7e73c
ed518fd
f9411d1
8218467
70d7275
d7bb353
c18dcda
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
<?php | ||
/** | ||
* Spacing block support flag. | ||
* Dimensions block support flag. | ||
* | ||
* @package gutenberg | ||
*/ | ||
|
@@ -10,21 +10,43 @@ | |
* | ||
* @param WP_Block_Type $block_type Block Type. | ||
*/ | ||
function gutenberg_register_spacing_support( $block_type ) { | ||
$has_spacing_support = gutenberg_block_has_support( $block_type, array( 'spacing' ), false ); | ||
|
||
function gutenberg_register_dimensions_support( $block_type ) { | ||
// Setup attributes and styles within that if needed. | ||
if ( ! $block_type->attributes ) { | ||
$block_type->attributes = array(); | ||
} | ||
|
||
if ( $has_spacing_support && ! array_key_exists( 'style', $block_type->attributes ) ) { | ||
// Check for existing style attribute definition e.g. from block.json. | ||
if ( array_key_exists( 'style', $block_type->attributes ) ) { | ||
return; | ||
} | ||
|
||
$has_spacing_support = gutenberg_block_has_support( $block_type, array( 'spacing' ), false ); | ||
// Future block supports such as height & width will be added here. | ||
|
||
if ( $has_spacing_support ) { | ||
$block_type->attributes['style'] = array( | ||
'type' => 'object', | ||
); | ||
} | ||
} | ||
|
||
/** | ||
* Add CSS classes for block dimensions to the incoming attributes array. | ||
* This will be applied to the block markup in the front-end. | ||
* | ||
* @param WP_Block_Type $block_type Block Type. | ||
* @param array $block_attributes Block attributes. | ||
* | ||
* @return array Block spacing CSS classes and inline styles. | ||
*/ | ||
function gutenberg_apply_dimensions_support( $block_type, $block_attributes ) { | ||
$spacing_styles = gutenberg_apply_spacing_support( $block_type, $block_attributes ); | ||
// Future block supports such as height and width will be added here. | ||
|
||
return $spacing_styles; | ||
} | ||
|
||
/** | ||
* Add CSS classes for block spacing to the incoming attributes array. | ||
* This will be applied to the block markup in the front-end. | ||
|
@@ -88,9 +110,9 @@ function gutenberg_skip_spacing_serialization( $block_type ) { | |
|
||
// Register the block support. | ||
WP_Block_Supports::get_instance()->register( | ||
'spacing', | ||
'dimensions', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the sort of thing I brought up at https://github.com/WordPress/gutenberg/pull/32499/files#r686747072 Isn't this problematic with existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might make more sense to keep things anchored to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation @nosolosw, I think I understand my error now in not appreciating that core also register support under the same key. I'll create a quick PR to change the key above from Regarding the comments on the PR introducing height block support I think there may be some confusion there, at least there is from my side, so I'll reply in more detail on that PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix is up in: #34030 Thank you for catching this one! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the quick turnaround, as always! |
||
array( | ||
'register_attribute' => 'gutenberg_register_spacing_support', | ||
'apply' => 'gutenberg_apply_spacing_support', | ||
'register_attribute' => 'gutenberg_register_dimensions_support', | ||
'apply' => 'gutenberg_apply_dimensions_support', | ||
) | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,154 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { | ||
__experimentalToolsPanel as ToolsPanel, | ||
__experimentalToolsPanelItem as ToolsPanelItem, | ||
} from '@wordpress/components'; | ||
import { Platform } from '@wordpress/element'; | ||
import { __ } from '@wordpress/i18n'; | ||
import { getBlockSupport } from '@wordpress/blocks'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import InspectorControls from '../components/inspector-controls'; | ||
import { | ||
MarginEdit, | ||
hasMarginSupport, | ||
hasMarginValue, | ||
resetMargin, | ||
useIsMarginDisabled, | ||
} from './margin'; | ||
import { | ||
PaddingEdit, | ||
hasPaddingSupport, | ||
hasPaddingValue, | ||
resetPadding, | ||
useIsPaddingDisabled, | ||
} from './padding'; | ||
import { cleanEmptyObject } from './utils'; | ||
|
||
export const SPACING_SUPPORT_KEY = 'spacing'; | ||
|
||
/** | ||
* Inspector controls for dimensions support. | ||
* | ||
* @param {Object} props Block props. | ||
* | ||
* @return {WPElement} Inspector controls for spacing support features. | ||
*/ | ||
export function DimensionsPanel( props ) { | ||
const isPaddingDisabled = useIsPaddingDisabled( props ); | ||
const isMarginDisabled = useIsMarginDisabled( props ); | ||
const isDisabled = useIsDimensionsDisabled( props ); | ||
const isSupported = hasDimensionsSupport( props.name ); | ||
|
||
if ( isDisabled || ! isSupported ) { | ||
return null; | ||
} | ||
|
||
const defaultSpacingControls = getBlockSupport( props.name, [ | ||
SPACING_SUPPORT_KEY, | ||
'__experimentalDefaultControls', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if we need this flag. Righ now blocks specify if some support is possible to use at all, then themes also specify if some UI is possible to use via theme.json if the UI is allowed by both block and theme it may show or not by default. Basically, we have 3 flags to enable a feature. I think the idea was two flags blocks specify if some UI appears or not, and themes may make the UI not appear by default, but even if the theme disables the UI to appear by default users would be able to enable by using the 3 dots. But I'm not sure if that's the plan or not maybe @mtias can confirm. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The addition of another type of flag isn't ideal, I agree. I'm not sure though that I agree with your suggestion, assuming I'm understanding it correctly. The theme.json allows for a theme developer to completely disable the UI for a particular type of block support feature. If the theme.json disables that UI, the user shouldn't have the ability to turn it on and use it anyway. That undermines the idea of disabling controls via theme.json. As it stands:
We need a means for the block to opt in, the theme.json to allow the UI but still hide the less important controls behind the If not a new flag, could we alter how the theme.json disables controls? The current boolean values to allow/disallow the UI would continue, but it could also support a third option to relegate a control to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, what I'm saying is making it impossible to totally disable UI functionality via theme.json it would only hide by default. I think I saw that discussed as a possibility in the past that's why I'm bringing it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I've missed that discussion around possibly moving away from theme.json having the final say in whether the UI is enabled or not. Do you have any links where I can find if a decision was made in that regard? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, unfortunately, I just have a vague idea of that option being considered in the past and I don't remember where and by who it was being considered. Maybe @mtias and/or @jasmussen may have better insights and confirm if this idea is a possibility or something we should not follow. |
||
] ); | ||
|
||
// Callback to reset all block support attributes controlled via this panel. | ||
const resetAll = () => { | ||
const { style } = props.attributes; | ||
|
||
props.setAttributes( { | ||
style: cleanEmptyObject( { | ||
...style, | ||
spacing: { | ||
...style?.spacing, | ||
margin: undefined, | ||
padding: undefined, | ||
}, | ||
} ), | ||
} ); | ||
}; | ||
|
||
return ( | ||
<InspectorControls key="dimensions"> | ||
<ToolsPanel | ||
label={ __( 'Dimensions options' ) } | ||
header={ __( 'Dimensions' ) } | ||
resetAll={ resetAll } | ||
> | ||
{ ! isPaddingDisabled && ( | ||
<ToolsPanelItem | ||
hasValue={ () => hasPaddingValue( props ) } | ||
label={ __( 'Padding' ) } | ||
onDeselect={ () => resetPadding( props ) } | ||
isShownByDefault={ defaultSpacingControls?.padding } | ||
> | ||
<PaddingEdit { ...props } /> | ||
</ToolsPanelItem> | ||
) } | ||
{ ! isMarginDisabled && ( | ||
<ToolsPanelItem | ||
hasValue={ () => hasMarginValue( props ) } | ||
label={ __( 'Margin' ) } | ||
onDeselect={ () => resetMargin( props ) } | ||
isShownByDefault={ defaultSpacingControls?.margin } | ||
> | ||
<MarginEdit { ...props } /> | ||
</ToolsPanelItem> | ||
) } | ||
</ToolsPanel> | ||
</InspectorControls> | ||
); | ||
} | ||
|
||
/** | ||
* Determine whether there is dimensions related block support. | ||
* | ||
* @param {string} blockName Block name. | ||
* | ||
* @return {boolean} Whether there is support. | ||
*/ | ||
export function hasDimensionsSupport( blockName ) { | ||
if ( Platform.OS !== 'web' ) { | ||
return false; | ||
} | ||
|
||
return hasPaddingSupport( blockName ) || hasMarginSupport( blockName ); | ||
} | ||
|
||
/** | ||
* Determines whether dimensions support has been disabled. | ||
* | ||
* @param {Object} props Block properties. | ||
* | ||
* @return {boolean} If spacing support is completely disabled. | ||
*/ | ||
const useIsDimensionsDisabled = ( props = {} ) => { | ||
const paddingDisabled = useIsPaddingDisabled( props ); | ||
const marginDisabled = useIsMarginDisabled( props ); | ||
|
||
return paddingDisabled && marginDisabled; | ||
}; | ||
|
||
/** | ||
* Custom hook to retrieve which padding/margin is supported | ||
* e.g. top, right, bottom or left. | ||
* | ||
* Sides are opted into by default. It is only if a specific side is set to | ||
* false that it is omitted. | ||
* | ||
* @param {string} blockName Block name. | ||
* @param {string} feature The feature custom sides relate to e.g. padding or margins. | ||
* | ||
* @return {Object} Sides supporting custom margin. | ||
*/ | ||
export function useCustomSides( blockName, feature ) { | ||
const support = getBlockSupport( blockName, SPACING_SUPPORT_KEY ); | ||
|
||
// Skip when setting is boolean as theme isn't setting arbitrary sides. | ||
if ( typeof support[ feature ] === 'boolean' ) { | ||
return; | ||
} | ||
|
||
return support[ feature ]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a second thought about this PR: I'm unsure whether we can rename a function that's been already shipped to WordPress core (see). Same for renaming the file:
spacing.php
todimensions.php
. These become "public APIs" once shipped and, generally, we can't remove them without notice: there's a deprecation process if need be, etc.Would like thoughts from folks with more experience working with WordPress core: @gziolo @jorgefilipecosta @youknowriad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it's similar to the above comment, even if this functions is marked "internal" in Core, so in theory we could rename, I think the file should be kept as
spacing.php
and the function renamed... Just stay consistent as long as the API is "spacing".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine using spacing for all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to rename this file back to
spacing.php
, restore the function name, and can do this tomorrow.I was under the impression we wanted to collect all the current spacing and proposed dimensions support under a "dimensions" toolset. Hence, the rename and combining of both here, my apologies it's missed the mark.
Would it be better to revert this file back to the original
spacing.php
and create a newdimensions.php
that will contain the height and width supports?The UI created via
dimensions.js
for the block editor anddimensions-panel.js
for site editor should be able to stay mostly as they are, correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be in the camp of using "Dimensions" for the UI labels... but keeping "spacing" for the code because of the backward compatibility concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to apologize for, that's what reviews are for!
Judging by the issues you linked, at the user/UI level, I presume the intention is to keep everything together, as you already did.
At the developer/code level, these are the boundaries that I see:
block.json
&theme.json
: as long as we use the same structure in both places, we should be fine. Either keeping everything under thespacing
namespace or usingspacing
for the existing (margin, padding) anddimensions
for the new (height, width).spacing
) or two (spacing
anddimensions
) should be fine as well, following what we decide for the keys.Within these boundaries, I believe the choices would be up to you. I can review that PR to help consolidate this code.