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

Extensibility: Convert all filters for components to behave like HOCs #3827

Merged
merged 8 commits into from
Dec 12, 2017
Merged

Extensibility: Convert all filters for components to behave like HOCs #3827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bf62d31
Component: Update withFilter HOC to support more advanced use cases
gziolo Dec 1, 2017
befeb75
Element: Update logic for getWrapperDisplayName method
gziolo Dec 1, 2017
d564276
Hooks: Update all namespaces to use slashes
gziolo Dec 5, 2017
81d56ad
Hooks: Convert all filters for components to behave like HOCs
gziolo Dec 6, 2017
eedd1ad
Hooks: Prefix all hook names with blocks
gziolo Dec 6, 2017
0ffd9ef
Hooks: Fix rerender issue with block component hooks
gziolo Dec 6, 2017
7bba1db
Editor: Update hooks integration for BlockListBlock component
gziolo Dec 8, 2017
a46d71a
Extensibility: Make sure all hooks are loaded before blocks are regis…
gziolo Dec 8, 2017
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
8 changes: 6 additions & 2 deletions blocks/api/registration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@
*/
import { get, isFunction, some } from 'lodash';

/**
* WordPress dependencies
*/
import { applyFilters } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import { applyFilters } from '../hooks';
import { getCategories } from './categories';

/**
Expand Down Expand Up @@ -115,7 +119,7 @@ export function registerBlockType( name, settings ) {
...settings,
};

settings = applyFilters( 'registerBlockType', settings, name );
settings = applyFilters( 'blocks.registerBlockType', settings, name );

return blocks[ name ] = settings;
}
Expand Down
4 changes: 2 additions & 2 deletions blocks/api/serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import { html as beautifyHtml } from 'js-beautify';
* WordPress dependencies
*/
import { Component, createElement, renderToString, cloneElement, Children } from '@wordpress/element';
import { applyFilters } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import { applyFilters } from '../hooks';
import { getBlockType, getUnknownTypeHandlerName } from './registration';

/**
Expand Down Expand Up @@ -55,7 +55,7 @@ export function getSaveContent( blockType, attributes ) {
}

// Applying the filters adding extra props
const props = applyFilters( 'getSaveContent.extraProps', { ...element.props }, blockType, attributes );
const props = applyFilters( 'blocks.getSaveContent.extraProps', { ...element.props }, blockType, attributes );

return cloneElement( element, props );
};
Expand Down
5 changes: 5 additions & 0 deletions blocks/api/test/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ describe( 'block parser', () => {
save: ( { attributes } ) => attributes.content,
};

beforeAll( () => {
// Load all hooks that modify blocks
require( 'blocks/hooks' );
} );

afterEach( () => {
setUnknownTypeHandlerName( undefined );
getBlockTypes().forEach( ( block ) => {
Expand Down
10 changes: 7 additions & 3 deletions blocks/block-edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
/**
* WordPress dependencies
*/
import { withFilters } from '@wordpress/components';

/**
* Internal dependencies
*/
import { applyFilters } from '../hooks';
import { getBlockType } from '../api';

function BlockEdit( props ) {
Expand All @@ -17,7 +21,7 @@ function BlockEdit( props ) {
// them preferencially as the render value for the block.
const Edit = blockType.edit || blockType.save;

return applyFilters( 'BlockEdit', <Edit key="edit" { ...editProps } />, props );
return <Edit { ...editProps } />;
}

export default BlockEdit;
export default withFilters( 'blocks.BlockEdit' )( BlockEdit );
35 changes: 19 additions & 16 deletions blocks/hooks/anchor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { assign } from 'lodash';
/**
* WordPress dependencies
*/
import { getWrapperDisplayName } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';
import { __ } from '@wordpress/i18n';

/**
Expand Down Expand Up @@ -46,33 +48,34 @@ export function addAttribute( settings ) {

/**
* Override the default edit UI to include a new block inspector control for
* assigning the anchor ID, if block supports anchor
* assigning the anchor ID, if block supports anchor.
*
* @param {Element} element Original edit element
* @param {Object} props Props passed to BlockEdit
* @return {Element} Filtered edit element
* @param {function|Component} BlockEdit Original component
* @return {function} Wrapped component
*/
export function addInspectorControl( element, props ) {
if ( hasBlockSupport( props.name, 'anchor' ) && props.focus ) {
element = [
element,
<InspectorControls key="inspector-anchor">
export function withInspectorControl( BlockEdit ) {
const WrappedBlockEdit = ( props ) => {
const hasAnchor = hasBlockSupport( props.name, 'anchor' ) && props.focus;

return [
<BlockEdit key="block-edit-anchor" { ...props } />,
hasAnchor && <InspectorControls key="inspector-anchor">
<InspectorControls.TextControl
label={ __( 'HTML Anchor' ) }
help={ __( 'Anchors lets you link directly to a section on a page.' ) }
value={ props.attributes.anchor || '' }
onChange={ ( nextValue ) => {
nextValue = nextValue.replace( ANCHOR_REGEX, '-' );

props.setAttributes( {
anchor: nextValue,
} );
} } />
</InspectorControls>,
];
}
};
WrappedBlockEdit.displayName = getWrapperDisplayName( BlockEdit, 'anchor' );

return element;
return WrappedBlockEdit;
}

/**
Expand All @@ -93,8 +96,8 @@ export function addSaveProps( extraProps, blockType, attributes ) {
return extraProps;
}

export default function anchor( { addFilter } ) {
addFilter( 'registerBlockType', 'core-anchor-attribute', addAttribute );
addFilter( 'BlockEdit', 'core-anchor-inspector-control', addInspectorControl );
addFilter( 'getSaveContent.extraProps', 'core-anchor-save-props', addSaveProps );
export default function anchor() {
addFilter( 'blocks.registerBlockType', 'core/anchor/attribute', addAttribute );
addFilter( 'blocks.BlockEdit', 'core/anchor/inspector-control', withInspectorControl );
addFilter( 'blocks.getSaveContent.extraProps', 'core/anchor/save-props', addSaveProps );
}
34 changes: 19 additions & 15 deletions blocks/hooks/custom-class-name.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { getWrapperDisplayName } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';
import { __ } from '@wordpress/i18n';

/**
Expand Down Expand Up @@ -37,17 +39,18 @@ export function addAttribute( settings ) {

/**
* Override the default edit UI to include a new block inspector control for
* assigning the anchor ID, if block supports anchor
* assigning the custom class name, if block supports custom class name.
*
* @param {Element} element Original edit element
* @param {Object} props Props passed to BlockEdit
* @return {Element} Filtered edit element
* @param {function|Component} BlockEdit Original component
* @return {function} Wrapped component
*/
export function addInspectorControl( element, props ) {
if ( hasBlockSupport( props.name, 'customClassName', true ) && props.focus ) {
element = [
element,
<InspectorControls key="inspector-custom-class-name">
export function withInspectorControl( BlockEdit ) {
const WrappedBlockEdit = ( props ) => {
const hasCustomClassName = hasBlockSupport( props.name, 'customClassName', true ) && props.focus;

return [
<BlockEdit key="block-edit-custom-class-name" { ...props } />,
hasCustomClassName && <InspectorControls key="inspector-custom-class-name">
<InspectorControls.TextControl
label={ __( 'Additional CSS Class' ) }
value={ props.attributes.className || '' }
Expand All @@ -59,9 +62,10 @@ export function addInspectorControl( element, props ) {
/>
</InspectorControls>,
];
}
};
WrappedBlockEdit.displayName = getWrapperDisplayName( BlockEdit, 'customClassName' );

return element;
return WrappedBlockEdit;
}

/**
Expand All @@ -82,8 +86,8 @@ export function addSaveProps( extraProps, blockType, attributes ) {
return extraProps;
}

export default function customClassName( { addFilter } ) {
addFilter( 'registerBlockType', 'core-custom-class-name-attribute', addAttribute );
addFilter( 'BlockEdit', 'core-custom-class-name-inspector-control', addInspectorControl );
addFilter( 'getSaveContent.extraProps', 'core-custom-class-name-save-props', addSaveProps );
export default function customClassName() {
addFilter( 'blocks.registerBlockType', 'core/custom-class-name/attribute', addAttribute );
Copy link
Member

Choose a reason for hiding this comment

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

I don't have strong opposition, but noting that there's a few implied conventions being introduced here:

  • Scoping filter names by with dot-separated namespaces
    • Any risk of confusion with other namespacing with /?
  • Multiple levels of namespacing, i.e. not just core/ but also core/custom-class-name/

Copy link
Member Author

Choose a reason for hiding this comment

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

Slash is not allowed for hook names... we need another patch if we want to change that.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can revisit namespaces, too. I’m fine with having only 2 first parts or open to other ideas.

addFilter( 'blocks.BlockEdit', 'core/custom-class-name/inspector-control', withInspectorControl );
addFilter( 'blocks.getSaveContent.extraProps', 'core/custom-class-name/save-props', addSaveProps );
}
9 changes: 7 additions & 2 deletions blocks/hooks/generated-class-name.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { addFilter } from '@wordpress/hooks';

/**
* Internal dependencies
*/
Expand Down Expand Up @@ -30,6 +35,6 @@ export function addGeneratedClassName( extraProps, blockType ) {
return extraProps;
}

export default function generatedClassName( { addFilter } ) {
addFilter( 'getSaveContent.extraProps', 'core-generated-class-name-save-props', addGeneratedClassName );
export default function generatedClassName() {
addFilter( 'blocks.getSaveContent.extraProps', 'core/generated-class-name/save-props', addGeneratedClassName );
}
35 changes: 4 additions & 31 deletions blocks/hooks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* WordPress dependencies
*/
import { createHooks } from '@wordpress/hooks';

/**
* Internal dependencies
*/
Expand All @@ -11,29 +6,7 @@ import customClassName from './custom-class-name';
import generatedClassName from './generated-class-name';
import matchers from './matchers';

const hooks = createHooks();

const {
addFilter,
removeFilter,
removeAllFilters,
applyFilters,
doingFilter,
didFilter,
hasFilter,
} = hooks;

export {
addFilter,
removeFilter,
removeAllFilters,
applyFilters,
doingFilter,
didFilter,
hasFilter,
};

anchor( hooks );
customClassName( hooks );
generatedClassName( hooks );
matchers( hooks );
anchor();
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Do we need to export these as functions anymore, or can they just call to addFilter when imported?

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 might not play well with unit tests. Need to verify.

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, we could export individual functions and apply filters when importing. I will open a follow-up so we could discuss it individually.

customClassName();
generatedClassName();
matchers();
9 changes: 7 additions & 2 deletions blocks/hooks/matchers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
*/
import { isFunction, mapValues } from 'lodash';

/**
* WordPress dependecies
*/
import { addFilter } from '@wordpress/hooks';

function warnAboutDeprecatedMatcher() {
// eslint-disable-next-line no-console
console.warn(
Expand Down Expand Up @@ -91,6 +96,6 @@ export function resolveAttributes( settings ) {
return settings;
}

export default function matchers( { addFilter } ) {
addFilter( 'registerBlockType', 'core-matchers', resolveAttributes );
export default function matchers() {
addFilter( 'blocks.registerBlockType', 'core/matchers', resolveAttributes );
}
14 changes: 6 additions & 8 deletions blocks/hooks/test/anchor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,17 @@ import { noop } from 'lodash';
/**
* WordPress dependencies
*/
import { createHooks } from '@wordpress/hooks';
import { applyFilters, removeAllFilters } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import anchor from '../anchor';

describe( 'anchor', () => {
const hooks = createHooks();

let blockSettings;
beforeEach( () => {
anchor( hooks );
anchor();

blockSettings = {
save: noop,
Expand All @@ -28,12 +26,12 @@ describe( 'anchor', () => {
} );

afterEach( () => {
hooks.removeAllFilters( 'registerBlockType' );
hooks.removeAllFilters( 'getSaveContent.extraProps' );
removeAllFilters( 'blocks.registerBlockType' );
removeAllFilters( 'blocks.getSaveContent.extraProps' );
} );

describe( 'addAttribute()', () => {
const addAttribute = hooks.applyFilters.bind( null, 'registerBlockType' );
const addAttribute = applyFilters.bind( null, 'blocks.registerBlockType' );

it( 'should do nothing if the block settings do not define anchor support', () => {
const settings = addAttribute( blockSettings );
Expand All @@ -54,7 +52,7 @@ describe( 'anchor', () => {
} );

describe( 'addSaveProps', () => {
const addSaveProps = hooks.applyFilters.bind( null, 'getSaveContent.extraProps' );
const addSaveProps = applyFilters.bind( null, 'blocks.getSaveContent.extraProps' );

it( 'should do nothing if the block settings do not define anchor support', () => {
const attributes = { anchor: 'foo' };
Expand Down
14 changes: 6 additions & 8 deletions blocks/hooks/test/custom-class-name.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,17 @@ import { noop } from 'lodash';
/**
* WordPress dependencies
*/
import { createHooks } from '@wordpress/hooks';
import { applyFilters, removeAllFilters } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import customClassName from '../custom-class-name';

describe( 'custom className', () => {
const hooks = createHooks();

let blockSettings;
beforeEach( () => {
customClassName( hooks );
customClassName();

blockSettings = {
save: noop,
Expand All @@ -28,12 +26,12 @@ describe( 'custom className', () => {
} );

afterEach( () => {
hooks.removeAllFilters( 'registerBlockType' );
hooks.removeAllFilters( 'getSaveContent.extraProps' );
removeAllFilters( 'blocks.registerBlockType' );
removeAllFilters( 'blocks.getSaveContent.extraProps' );
} );

describe( 'addAttribute()', () => {
const addAttribute = hooks.applyFilters.bind( null, 'registerBlockType' );
const addAttribute = applyFilters.bind( null, 'blocks.registerBlockType' );

it( 'should do nothing if the block settings disable custom className support', () => {
const settings = addAttribute( {
Expand All @@ -54,7 +52,7 @@ describe( 'custom className', () => {
} );

describe( 'addSaveProps', () => {
const addSaveProps = hooks.applyFilters.bind( null, 'getSaveContent.extraProps' );
const addSaveProps = applyFilters.bind( null, 'blocks.getSaveContent.extraProps' );

it( 'should do nothing if the block settings do not define custom className support', () => {
const attributes = { className: 'foo' };
Expand Down
Loading