Skip to content
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

Merged
merged 50 commits into from
Aug 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
795fe99
Add utils to spacing supports to check or reset values
aaronrobertshaw May 31, 2021
dfb1403
Draft block support panel
aaronrobertshaw May 31, 2021
0cb9f82
Update spacing hook to use new block support panel
aaronrobertshaw May 31, 2021
1b23fad
Update block support panel to handle false children
aaronrobertshaw Jun 1, 2021
2487805
Conditionally display spacing block support controls
aaronrobertshaw Jun 1, 2021
696070f
Draft README for BlockSupportPanel
aaronrobertshaw Jun 1, 2021
03c87c4
Make block supports panel handle filtered children
aaronrobertshaw Jun 1, 2021
5fa856f
Add initial tests for block supports panel
aaronrobertshaw Jun 1, 2021
7fb7e8a
Add story for block support panel
aaronrobertshaw Jun 1, 2021
62d7895
Rename spacing support to dimensions
aaronrobertshaw Jun 2, 2021
5f7b39f
Rename spacing support to dimensions in FSE
aaronrobertshaw Jun 2, 2021
db0c413
Update GlobalStyles dimensions panel
aaronrobertshaw Jun 2, 2021
dd015b0
Add means to handle default controls in block support panel
aaronrobertshaw Jun 4, 2021
659a79e
Change default controls display in block supports panel
aaronrobertshaw Jun 7, 2021
213d250
Make default controls still show after reset all
aaronrobertshaw Jun 7, 2021
a96dc35
Change back to default controls always displaying
aaronrobertshaw Jun 9, 2021
481ea39
Tweak panel to make it a little more generic
aaronrobertshaw Jun 23, 2021
0d75ba7
Simplify progressive disclosure panel state
aaronrobertshaw Jul 8, 2021
ffedb61
Add progressive disclosure panel item component
aaronrobertshaw Jul 8, 2021
ae4a3b2
Update dimensions global style sidebar panel
aaronrobertshaw Jul 8, 2021
e7a89ea
Update docs and comments after restructure
aaronrobertshaw Jul 12, 2021
07420bc
Update panel story with new item component
aaronrobertshaw Jul 12, 2021
9ec400f
Update tests to handle new panel item component
aaronrobertshaw Jul 12, 2021
a4d96b0
Clean new style objects after resetting spacing values
aaronrobertshaw Jul 21, 2021
17bd1b6
Disable default control menu items when they have no value
aaronrobertshaw Jul 22, 2021
4e33e6c
Add test for disabling default control menu items
aaronrobertshaw Jul 22, 2021
3609b5a
Add cleanEmptyObject to dimensions resetAll
aaronrobertshaw Jul 27, 2021
5746e25
Switch panel to grid layout for future multi-column layout
aaronrobertshaw Jul 27, 2021
c8d96ee
Tweak defaults for controls spacing in progressive disclosure panel
aaronrobertshaw Jul 28, 2021
7c22312
Fix Global Styles sidebar panel styling
aaronrobertshaw Jul 28, 2021
dbaa31c
Handle cases where panel item's inner element returns null
aaronrobertshaw Jul 29, 2021
8f196bc
Correct component name in readme
aaronrobertshaw Jul 30, 2021
76a2b8d
Update readme with experimental alert and improved prop format
aaronrobertshaw Jul 30, 2021
224c0ea
Fix menuItem child filtering using Children util.
aaronrobertshaw Jul 30, 2021
8343c9b
Accurately name the ProgressiveDisclosurePanelItem in tests
aaronrobertshaw Jul 30, 2021
6068bec
Initial pass at restructuring component
aaronrobertshaw Jul 30, 2021
48388f5
Rename title component and props to header
aaronrobertshaw Jul 30, 2021
92bcf52
Change progressive panel items to register themselves
aaronrobertshaw Aug 2, 2021
a624202
Change menu item selection state to enum
aaronrobertshaw Aug 2, 2021
eaad38e
Improve tests and rename to ToolsPanel
aaronrobertshaw Aug 3, 2021
8a7df11
Refactor panel context to be consistent with other components
aaronrobertshaw Aug 3, 2021
ccee3ac
Extract menu states to utils
aaronrobertshaw Aug 3, 2021
f2851b6
Fix caching of block attributes via callbacks stored in ToolsPanel state
aaronrobertshaw Aug 4, 2021
3d7e73c
Move execution of panel item callbacks into the item on menu toggle
aaronrobertshaw Aug 4, 2021
ed518fd
Convert to styled components and connect to context system
aaronrobertshaw Aug 5, 2021
f9411d1
Limit component.js files to presentation
aaronrobertshaw Aug 5, 2021
8218467
Switch approach back to being able to hide default controls
aaronrobertshaw Aug 6, 2021
70d7275
Tweak storybook entry for ToolsPanel
aaronrobertshaw Aug 6, 2021
d7bb353
Highlight experimental nature of component in story title
aaronrobertshaw Aug 10, 2021
c18dcda
Fix typo
aaronrobertshaw Aug 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions docs/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -1265,6 +1265,24 @@
"markdown_source": "../packages/components/src/toolbar/README.md",
"parent": "components"
},
{
"title": "ToolsPanelHeader",
"slug": "tools-panel-header",
"markdown_source": "../packages/components/src/tools-panel/tools-panel-header/README.md",
"parent": "components"
},
{
"title": "ToolsPanelItem",
"slug": "tools-panel-item",
"markdown_source": "../packages/components/src/tools-panel/tools-panel-item/README.md",
"parent": "components"
},
{
"title": "ToolsPanel",
"slug": "tools-panel",
"markdown_source": "../packages/components/src/tools-panel/tools-panel/README.md",
"parent": "components"
},
{
"title": "Tooltip",
"slug": "tooltip",
Expand Down
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
*/
Expand All @@ -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 ) {
Copy link
Member

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 to dimensions.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

Copy link
Contributor

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".

Copy link
Member

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.

Copy link
Contributor Author

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 new dimensions.php that will contain the height and width supports?

The UI created via dimensions.js for the block editor and dimensions-panel.js for site editor should be able to stay mostly as they are, correct?

Copy link
Contributor

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.

Copy link
Member

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:

  • Keys in block.json & theme.json: as long as we use the same structure in both places, we should be fine. Either keeping everything under the spacing namespace or using spacing for the existing (margin, padding) and dimensions for the new (height, width).
  • Server hook: either having a single hook (spacing) or two (spacing and dimensions) should be fine as well, following what we decide for the keys.
  • Client hook: keeping a single UI hook to gather all props together looks simpler than trying to use SlotFills for this. Unlike the server code, we don't export the client hooks or the functions affected by this change, so keep the name "dimensions" instead of "spacing" is a style choice — it also matches well with what we use at the user level.

Within these boundaries, I believe the choices would be up to you. I can review that PR to help consolidate this code.

// 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.
Expand Down Expand Up @@ -88,9 +110,9 @@ function gutenberg_skip_spacing_serialization( $block_type ) {

// Register the block support.
WP_Block_Supports::get_instance()->register(
'spacing',
'dimensions',
Copy link
Member

Choose a reason for hiding this comment

The 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 spacing supports? What happens when core (WordPress 5.8) registers support for spacing and the plugin does not overwrite it as before but instead registers a different thing called dimensions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make more sense to keep things anchored to spacing for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 dimensions back to spacing and add an inline comment explaining why it is still spacing.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix is up in: #34030

Thank you for catching this one!

Copy link
Member

Choose a reason for hiding this comment

The 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',
)
);
2 changes: 1 addition & 1 deletion lib/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -128,5 +128,5 @@ function gutenberg_is_experiment_enabled( $name ) {
require __DIR__ . '/block-supports/custom-classname.php';
require __DIR__ . '/block-supports/border.php';
require __DIR__ . '/block-supports/layout.php';
require __DIR__ . '/block-supports/spacing.php';
require __DIR__ . '/block-supports/dimensions.php';
require __DIR__ . '/block-supports/duotone.php';
154 changes: 154 additions & 0 deletions packages/block-editor/src/hooks/dimensions.js
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',
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

  • Block.json --> Allows opt-in for support feature
  • Theme.json --> Allows for the UI to be disabled

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 ... menu.

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 ... menu. I'm not sold on that approach either so very open to fresh ideas.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the theme.json disables that UI, the user shouldn't have the ability to turn it on and use it anyway

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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?

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 ];
}
2 changes: 1 addition & 1 deletion packages/block-editor/src/hooks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import './font-size';
import './border-color';
import './layout';

export { useCustomSides } from './spacing';
export { useCustomSides } from './dimensions';
export { getBorderClassesAndStyles, useBorderProps } from './use-border-props';
export { getColorClassesAndStyles, useColorProps } from './use-color-props';
export { getSpacingClassesAndStyles } from './use-spacing-props';
35 changes: 34 additions & 1 deletion packages/block-editor/src/hooks/margin.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
* Internal dependencies
*/
import useSetting from '../components/use-setting';
import { SPACING_SUPPORT_KEY, useCustomSides } from './spacing';
import { SPACING_SUPPORT_KEY, useCustomSides } from './dimensions';
import { cleanEmptyObject } from './utils';

/**
Expand All @@ -28,6 +28,38 @@ export function hasMarginSupport( blockType ) {
return !! ( true === support || support?.margin );
}

/**
* Checks if there is a current value in the margin block support attributes.
*
* @param {Object} props Block props.
* @return {boolean} Whether or not the block has a margin value set.
*/
export function hasMarginValue( props ) {
return props.attributes.style?.spacing?.margin !== undefined;
}

/**
* Resets the margin block support attributes. This can be used when disabling
* the margin support controls for a block via a `ToolsPanel`.
*
* @param {Object} props Block props.
* @param {Object} props.attributes Block's attributes.
* @param {Object} props.setAttributes Function to set block's attributes.
*/
export function resetMargin( { attributes = {}, setAttributes } ) {
const { style } = attributes;

setAttributes( {
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
style: cleanEmptyObject( {
...style,
spacing: {
...style?.spacing,
margin: undefined,
},
} ),
} );
}

/**
* Custom hook that checks if margin settings have been disabled.
*
Expand Down Expand Up @@ -106,6 +138,7 @@ export function MarginEdit( props ) {
label={ __( 'Margin' ) }
sides={ sides }
units={ units }
allowReset={ false }
/>
</>
),
Expand Down
35 changes: 34 additions & 1 deletion packages/block-editor/src/hooks/padding.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
* Internal dependencies
*/
import useSetting from '../components/use-setting';
import { SPACING_SUPPORT_KEY, useCustomSides } from './spacing';
import { SPACING_SUPPORT_KEY, useCustomSides } from './dimensions';
import { cleanEmptyObject } from './utils';

/**
Expand All @@ -28,6 +28,38 @@ export function hasPaddingSupport( blockType ) {
return !! ( true === support || support?.padding );
}

/**
* Checks if there is a current value in the padding block support attributes.
*
* @param {Object} props Block props.
* @return {boolean} Whether or not the block has a padding value set.
*/
export function hasPaddingValue( props ) {
return props.attributes.style?.spacing?.padding !== undefined;
}

/**
* Resets the padding block support attributes. This can be used when disabling
* the padding support controls for a block via a `ToolsPanel`.
*
* @param {Object} props Block props.
* @param {Object} props.attributes Block's attributes.
* @param {Object} props.setAttributes Function to set block's attributes.
*/
export function resetPadding( { attributes = {}, setAttributes } ) {
const { style } = attributes;

setAttributes( {
style: cleanEmptyObject( {
...style,
spacing: {
...style?.spacing,
padding: undefined,
},
} ),
} );
}

/**
* Custom hook that checks if padding settings have been disabled.
*
Expand Down Expand Up @@ -106,6 +138,7 @@ export function PaddingEdit( props ) {
label={ __( 'Padding' ) }
sides={ sides }
units={ units }
allowReset={ false }
/>
</>
),
Expand Down
Loading