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.

Sign up for GitHub

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

Jump to bottom

Block editor: new editor.BlockControls filter #48809

Closed
wants to merge 7 commits into from
Closed

Block editor: new editor.BlockControls filter #48809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c5abcef
Block editor: dedicated block controls filter
ellatrix Mar 6, 2023
56c6679
Rerender on hook add/remove
ellatrix Mar 6, 2023
2b6f49e
Add support key conditions
ellatrix Mar 7, 2023
b2f240a
Forgot dependencies
ellatrix Mar 7, 2023
724a151
Make filter key private
ellatrix Mar 7, 2023
49e6aa6
Make sure the hook name starts with a letter
ellatrix Mar 7, 2023
afd49e8
Fix unit tests
ellatrix Mar 7, 2023
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
3 changes: 2 additions & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion packages/block-editor/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@
"react-easy-crop": "^4.5.1",
"rememo": "^4.0.0",
"remove-accents": "^0.4.2",
"traverse": "^0.6.6"
"traverse": "^0.6.6",
"uuid": "8.3.0"
},
"peerDependencies": {
"react": "^18.0.0",
Expand Down
85 changes: 84 additions & 1 deletion packages/block-editor/src/components/block-edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
/**
* External dependencies
*/
import { v4 as uuid } from 'uuid';

/**
* WordPress dependencies
*/
import { useMemo } from '@wordpress/element';
import { useMemo, useEffect, useReducer } from '@wordpress/element';

import { hasBlockSupport } from '@wordpress/blocks';
import { filters, addAction, removeAction } from '@wordpress/hooks';
import { useSelect } from '@wordpress/data';

/**
* Internal dependencies
*/
import Edit from './edit';
import { BlockEditContextProvider, useBlockEditContext } from './context';
import { store as blockEditorStore } from '../../store';

/**
* The `useBlockEditContext` hook provides information about the block this hook is being used in.
Expand All @@ -20,6 +29,79 @@ import { BlockEditContextProvider, useBlockEditContext } from './context';
*/
export { useBlockEditContext };

// Please do not export this at the package level until we have a stable API.
// Hook names must start with a letter.
export const blockControlsFilterName = 'a' + uuid();

function BlockControlFilters( props ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this the same thing as just adding an empty component and wrapping it with withFilters or something like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there are a few differences:

  • The filter doesn't have to render the previous value. This is a great improvement DevX wise.
  • It adds a check shouldDisplayControls: I'm guessing that's to improve performance and avoid rendering these controls if not necessary. While this is a good thing probably, it does add a bit of overhead in the sense that most of these filters actually render InspectorControls or BlockControls that do these checks as well. So using a context value for these checks might still be a good idea. Also, should we support the case where we pass flag to enable __experimentalExposeControlsToChildren behavior or not? (so let's say that this is generally a good thing)
  • The other thing is that folks can render anything in this filter and not just controls, like random Divs... So maybe, we should forbid that by adding some hidden wrapper or something like that or finding a way to check that it renders no DOM nodes.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the performance aspect, we should be a bit careful here to not actually degrade performance. Some existing hooks might have existed earlier (like by checking a block support) while here we first check shouldDisplayControls then check the block supports ... within the hook. So I wonder if we should first land shouldDisplayControls as a context value before this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

While this is a good thing probably, it does add a bit of overhead in the sense that most of these filters actually render InspectorControls or BlockControls that do these checks as well. So using a context value for these checks might still be a good idea.

The overhead is almost negligible. I'd prefer keeping all this internal instead of exposing it through context, although this context in particular can be entirely internal if we don't use the existing context.

Also, should we support the case where we pass flag to enable __experimentalExposeControlsToChildren behavior or not? (so let's say that this is generally a good thing)

It supports it. The filters will render.

For the performance aspect, we should be a bit careful here to not actually degrade performance. Some existing hooks might have existed earlier (like by checking a block support) while here we first check shouldDisplayControls then check the block supports ... within the hook. So I wonder if we should first land shouldDisplayControls as a context value before this PR.

Not sure how this would degrade performance. We still guard hasSelectedInnerBlock with a __experimentalExposeControlsToChildren check. Using context for InspectorControls and BlockControls would be better yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

The filter doesn't have to render the previous value. This is a great improvement DevX wise.

That's correct. If this is a good pattern for other filters, at some point we could move this to the components package next to withFilters, but let's see how it goes.

Copy link
Member Author

Choose a reason for hiding this comment

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

The other thing is that folks can render anything in this filter and not just controls, like random Divs... So maybe, we should forbid that by adding some hidden wrapper or something like that or finding a way to check that it renders no DOM nodes.

Yes, they could do this previously too. I do like that idea! We should be as restrictive as possible with this new filter.

Copy link
Contributor

Choose a reason for hiding this comment

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

It supports it. The filters will render.

I mean some controls won't render if the block itself is not selected and that additional check and rendering is not necessary in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean some controls won't render if the block itself is not selected and that additional check and rendering is not necessary in this case.

I'm not sure I'm following 😅

const { name, isSelected, clientId } = props;
const shouldDisplayControls = useSelect(
( select ) => {
if ( isSelected ) {
return true;
}

const {
getBlockName,
isFirstMultiSelectedBlock,
getMultiSelectedBlockClientIds,
hasSelectedInnerBlock,
} = select( blockEditorStore );

if ( isFirstMultiSelectedBlock( clientId ) ) {
return getMultiSelectedBlockClientIds().every(
( id ) => getBlockName( id ) === name
);
}

return (
hasBlockSupport(
getBlockName( clientId ),
'__experimentalExposeControlsToChildren',
false
) && hasSelectedInnerBlock( clientId )
);
},
[ clientId, isSelected, name ]
);

const [ , forceRender ] = useReducer( () => [] );

useEffect( () => {
const namespace = 'core/block-edit/block-controls';

function onHooksUpdated( updatedHookName ) {
if ( updatedHookName === blockControlsFilterName ) {
forceRender();
}
}

addAction( 'hookRemoved', namespace, onHooksUpdated );
addAction( 'hookAdded', namespace, onHooksUpdated );

return () => {
removeAction( 'hookRemoved', namespace );
removeAction( 'hookAdded', namespace );
};
}, [] );

if ( ! shouldDisplayControls ) {
return;
}

const blockControlFilters = filters[ blockControlsFilterName ];

if ( ! blockControlFilters ) {
return;
}

return blockControlFilters.handlers.map(
Copy link
Member

@gziolo gziolo Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's probably not quite how WordPress filters were designed to work. I'm not sure I've ever seen usage where the previous value would not get set at all. It works here really well though 😄

( { callback: Controls, namespace } ) => {
return <Controls { ...props } key={ namespace } />;
}
);
}

export default function BlockEdit( props ) {
const {
name,
Expand Down Expand Up @@ -48,6 +130,7 @@ export default function BlockEdit( props ) {
// See https://reactjs.org/docs/context.html#caveats.
value={ useMemo( () => context, Object.values( context ) ) }
>
<BlockControlFilters { ...props } />
<Edit { ...props } />
</BlockEditContextProvider>
);
Expand Down
110 changes: 52 additions & 58 deletions packages/block-editor/src/hooks/align.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { createHigherOrderComponent } from '@wordpress/compose';
import { createHigherOrderComponent, ifCondition } from '@wordpress/compose';
import { addFilter } from '@wordpress/hooks';
import {
getBlockSupport,
Expand All @@ -21,6 +21,7 @@ import { useSelect } from '@wordpress/data';
import { BlockControls, BlockAlignmentControl } from '../components';
import useAvailableAlignments from '../components/block-alignment-control/use-available-alignments';
import { store as blockEditorStore } from '../store';
import { blockControlsFilterName } from '../components/block-edit';

/**
* An array which includes all possible valid alignments,
Expand Down Expand Up @@ -113,64 +114,55 @@ export function addAttribute( settings ) {
* Override the default edit UI to include new toolbar controls for block
* alignment, if block defines support.
*
* @param {Function} BlockEdit Original component.
*
* @return {Function} Wrapped component.
* @param {Object} props
*/
export const withToolbarControls = createHigherOrderComponent(
( BlockEdit ) => ( props ) => {
const blockEdit = <BlockEdit key="edit" { ...props } />;
const { name: blockName } = props;
// Compute the block valid alignments by taking into account,
// if the theme supports wide alignments or not and the layout's
// availble alignments. We do that for conditionally rendering
// Slot.
const blockAllowedAlignments = getValidAlignments(
getBlockSupport( blockName, 'align' ),
hasBlockSupport( blockName, 'alignWide', true )
);

const validAlignments = useAvailableAlignments(
blockAllowedAlignments
).map( ( { name } ) => name );
const isContentLocked = useSelect(
( select ) => {
return select(
blockEditorStore
).__unstableGetContentLockingParent( props.clientId );
},
[ props.clientId ]
);
if ( ! validAlignments.length || isContentLocked ) {
return blockEdit;
}
export const ToolbarControls = ( props ) => {
const { name: blockName } = props;
// Compute the block valid alignments by taking into account,
// if the theme supports wide alignments or not and the layout's
// availble alignments. We do that for conditionally rendering
// Slot.
const blockAllowedAlignments = getValidAlignments(
getBlockSupport( blockName, 'align' ),
hasBlockSupport( blockName, 'alignWide', true )
);

const validAlignments = useAvailableAlignments(
blockAllowedAlignments
).map( ( { name } ) => name );
const isContentLocked = useSelect(
( select ) => {
return select( blockEditorStore ).__unstableGetContentLockingParent(
props.clientId
);
},
[ props.clientId ]
);
if ( ! validAlignments.length || isContentLocked ) {
return null;
}

const updateAlignment = ( nextAlign ) => {
if ( ! nextAlign ) {
const blockType = getBlockType( props.name );
const blockDefaultAlign = blockType?.attributes?.align?.default;
if ( blockDefaultAlign ) {
nextAlign = '';
}
const updateAlignment = ( nextAlign ) => {
if ( ! nextAlign ) {
const blockType = getBlockType( props.name );
const blockDefaultAlign = blockType?.attributes?.align?.default;
if ( blockDefaultAlign ) {
nextAlign = '';
}
props.setAttributes( { align: nextAlign } );
};

return (
<>
<BlockControls group="block" __experimentalShareWithChildBlocks>
<BlockAlignmentControl
value={ props.attributes.align }
onChange={ updateAlignment }
controls={ validAlignments }
/>
</BlockControls>
{ blockEdit }
</>
);
},
'withToolbarControls'
);
}
props.setAttributes( { align: nextAlign } );
};

return (
<BlockControls group="block" __experimentalShareWithChildBlocks>
<BlockAlignmentControl
value={ props.attributes.align }
onChange={ updateAlignment }
controls={ validAlignments }
/>
</BlockControls>
);
};

/**
* Override the default block element to add alignment wrapper props.
Expand Down Expand Up @@ -248,9 +240,11 @@ addFilter(
withDataAlign
);
addFilter(
'editor.BlockEdit',
blockControlsFilterName,
'core/editor/align/with-toolbar-controls',
withToolbarControls
ifCondition( ( { name } ) => hasBlockSupport( name, 'align' ) )(
ToolbarControls
)
Copy link
Member Author

Choose a reason for hiding this comment

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

It's worth noting that these checks aren't contributing much to the performance improvement, but still, it's good practice to avoid as much calls to hooks as possible. Hopefully new hooks and plugins copy this pattern.

);
addFilter(
'blocks.getSaveContent.extraProps',
Expand Down
Loading