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

Try: Extensibility: Define anchor behavior as filtered blocks support #3318

Merged
merged 3 commits into from
Nov 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 0 additions & 5 deletions blocks/api/factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,6 @@ export function createBlock( name, blockAttributes = {} ) {
return result;
}, {} );

// Keep the anchor if the block supports it
if ( blockType.supportAnchor && blockAttributes.anchor ) {
attributes.anchor = blockAttributes.anchor;
}

// Keep the className if the block supports it
if ( blockType.className !== false && blockAttributes.className ) {
attributes.className = blockAttributes.className;
Expand Down
1 change: 1 addition & 0 deletions blocks/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ export {
getDefaultBlockName,
getBlockType,
getBlockTypes,
hasBlockSupport,
} from './registration';
7 changes: 1 addition & 6 deletions blocks/api/parser.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { parse as hpqParse, attr } from 'hpq';
import { parse as hpqParse } from 'hpq';
import { mapValues, reduce, pickBy } from 'lodash';

/**
Expand Down Expand Up @@ -148,11 +148,6 @@ export function getBlockAttributes( blockType, innerHTML, attributes ) {
return result;
}, {} );

// If the block supports anchor, parse the id
if ( blockType.supportAnchor ) {
blockAttributes.anchor = hpqParse( innerHTML, attr( '*', 'id' ) );
}

// If the block supports a custom className parse it
if ( blockType.className !== false && attributes && attributes.className ) {
blockAttributes.className = attributes.className;
Expand Down
31 changes: 29 additions & 2 deletions blocks/api/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import { get, isFunction, some } from 'lodash';
*/
import { getCategories } from './categories';

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

/**
* Block settings keyed by block name.
*
Expand Down Expand Up @@ -113,13 +118,15 @@ export function registerBlockType( name, settings ) {
if ( ! settings.icon ) {
settings.icon = 'block-default';
}
const block = blocks[ name ] = {
settings = {
name,
attributes: get( window._wpBlocksAttributes, name ),
...settings,
};

return block;
settings = applyFilters( 'registerBlockType', settings, name );
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we revert the arguments to match the registerBlockType function signature? I know it's not convenient for the anchor filter because the name is useless.

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 awkward, I agree, but the behavior of filtering is such that we need the second argument to be the original value we expect to be filtered; in this case, the settings. All additional arguments are considered "extra", and in this case the name could prove valuable and is not otherwise made available.

See: https://developer.wordpress.org/reference/functions/apply_filters/


return blocks[ name ] = settings;
}

/**
Expand Down Expand Up @@ -196,3 +203,23 @@ export function getBlockType( name ) {
export function getBlockTypes() {
return Object.values( blocks );
}

/**
* Returns true if the block defines support for a feature, or false otherwise
*
* @param {(String|Object)} nameOrType Block name or type object
* @param {String} feature Feature to test
* @param {Boolean} defaultSupports Whether feature is supported by
* default if not explicitly defined
* @return {Boolean} Whether block supports feature
*/
export function hasBlockSupport( nameOrType, feature, defaultSupports ) {
const blockType = 'string' === typeof nameOrType ?
getBlockType( nameOrType ) :
nameOrType;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: but I tend to prefer functions with an unchanged signature. My preference would be to always use type and call getBlockType on the caller side when needed.

Copy link
Member

@gziolo gziolo Nov 14, 2017

Choose a reason for hiding this comment

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

It also makes documenting easier when you enforce calling getBlockType explicitly. We can always provide two methods to support those 2 different ways of calling:

export function hasBlockTypeSupport( type, feature, defaultSupports ) {}

export function hasBlockSupport( name, feature, defaultSupports ) {
    return hasBlockTypeSupport( getBlockType( name ), feature,  defaultSupports );
}

The only challenge in such case is to find proper names for methods :)


return !! get( blockType, [
'supports',
feature,
], defaultSupports );
}
7 changes: 2 additions & 5 deletions blocks/api/serializer.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { Component, createElement, renderToString, cloneElement, Children } from
* Internal dependencies
*/
import { getBlockType, getUnknownTypeHandlerName } from './registration';
import { applyFilters } from '../hooks';

/**
* Returns the block's default classname from its name
Expand Down Expand Up @@ -55,7 +56,7 @@ export function getSaveContent( blockType, attributes ) {
return element;
}

const extraProps = {};
const extraProps = applyFilters( 'getSaveContent.extraProps', {}, blockType, attributes );
Copy link
Member

Choose a reason for hiding this comment

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

This one seems to be a very specific. Maybe it should be simply called addAdvancedAttributes instead of getSaveContent.extraProps to avoid dot notation in the name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I think getSaveContent.extraProps convey the correct meaning. I don't care much about the dot notation

if ( !! className ) {
const updatedClassName = classnames(
className,
Expand All @@ -65,10 +66,6 @@ export function getSaveContent( blockType, attributes ) {
extraProps.className = updatedClassName;
}

if ( blockType.supportAnchor && attributes.anchor ) {
extraProps.id = attributes.anchor;
}

return cloneElement( element, extraProps );
};
const contentWithClassname = Children.map( saveContent, addAdvancedAttributes );
Expand Down
24 changes: 0 additions & 24 deletions blocks/api/test/factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,30 +57,6 @@ describe( 'block factory', () => {
expect( typeof block.uid ).toBe( 'string' );
} );

it( 'should keep the anchor if the block supports it', () => {
registerBlockType( 'core/test-block', {
attributes: {
align: {
type: 'string',
},
},
save: noop,
category: 'common',
title: 'test block',
supportAnchor: true,
} );
const block = createBlock( 'core/test-block', {
align: 'left',
anchor: 'chicken',
} );

expect( block.attributes ).toEqual( {
anchor: 'chicken',
align: 'left',
} );
expect( block.isValid ).toBe( true );
} );

it( 'should keep the className if the block supports it', () => {
registerBlockType( 'core/test-block', {
attributes: {},
Expand Down
20 changes: 0 additions & 20 deletions blocks/api/test/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,26 +157,6 @@ describe( 'block parser', () => {
} );
} );

it( 'should parse the anchor if the block supports it', () => {
const blockType = {
attributes: {
content: {
type: 'string',
source: text( 'div' ),
},
},
supportAnchor: true,
};

const innerHTML = '<div id="chicken">Ribs</div>';
const attrs = {};

expect( getBlockAttributes( blockType, innerHTML, attrs ) ).toEqual( {
content: 'Ribs',
anchor: 'chicken',
} );
} );

it( 'should parse the className if the block supports it', () => {
const blockType = {
attributes: {},
Expand Down
64 changes: 64 additions & 0 deletions blocks/api/test/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
getDefaultBlockName,
getBlockType,
getBlockTypes,
hasBlockSupport,
} from '../registration';

describe( 'blocks', () => {
Expand Down Expand Up @@ -272,4 +273,67 @@ describe( 'blocks', () => {
] );
} );
} );

describe( 'hasBlockSupport', () => {
it( 'should return false if block has no supports', () => {
registerBlockType( 'core/test-block', defaultBlockSettings );

expect( hasBlockSupport( 'core/test-block', 'foo' ) ).toBe( false );
} );

it( 'should return false if block does not define support by name', () => {
registerBlockType( 'core/test-block', {
...defaultBlockSettings,
supports: {
bar: true,
},
} );

expect( hasBlockSupport( 'core/test-block', 'foo' ) ).toBe( false );
} );

it( 'should return custom default supports if block does not define support by name', () => {
registerBlockType( 'core/test-block', {
...defaultBlockSettings,
supports: {
bar: true,
},
} );

expect( hasBlockSupport( 'core/test-block', 'foo', true ) ).toBe( true );
} );

it( 'should return true if block type supports', () => {
registerBlockType( 'core/test-block', {
...defaultBlockSettings,
supports: {
foo: true,
},
} );

expect( hasBlockSupport( 'core/test-block', 'foo' ) ).toBe( true );
} );

it( 'should return true if block author defines unsupported but truthy value', () => {
registerBlockType( 'core/test-block', {
...defaultBlockSettings,
supports: {
foo: 'hmmm',
},
} );

expect( hasBlockSupport( 'core/test-block', 'foo' ) ).toBe( true );
} );

it( 'should handle block settings object as argument to test', () => {
const settings = {
...defaultBlockSettings,
supports: {
foo: true,
},
};

expect( hasBlockSupport( settings, 'foo' ) ).toBe( true );
} );
} );
} );
14 changes: 0 additions & 14 deletions blocks/api/test/serializer.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,20 +121,6 @@ describe( 'block serializer', () => {

expect( saved ).toBe( '<div>Bananas</div>' );
} );

it( 'should add an id if the block supports anchors', () => {
const saved = getSaveContent(
{
save: ( { attributes } ) => createElement( 'div', null, attributes.fruit ),
supportAnchor: true,
name: 'myplugin/fruit',
className: false,
},
{ fruit: 'Bananas', anchor: 'my-fruit' }
);

expect( saved ).toBe( '<div id="my-fruit">Bananas</div>' );
} );
} );

describe( 'component save', () => {
Expand Down
26 changes: 26 additions & 0 deletions blocks/block-edit/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* Internal dependencies
*/
import { getBlockType } from '../api';
import { applyFilters } from '../hooks';

function BlockEdit( props ) {
const { name, ...editProps } = props;
const blockType = getBlockType( name );

if ( ! blockType ) {
return null;
}

// `edit` and `save` are functions or components describing the markup
// with which a block is displayed. If `blockType` is valid, assign
// them preferencially as the render value for the block.
let Edit;
if ( blockType ) {
Edit = blockType.edit || blockType.save;
}

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

export default BlockEdit;
56 changes: 56 additions & 0 deletions blocks/block-edit/test/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/**
* External dependencies
*/
import { shallow } from 'enzyme';
import { noop } from 'lodash';

/**
* Internal dependencies
*/
import BlockEdit from '../';
import {
registerBlockType,
unregisterBlockType,
getBlockTypes,
} from '../../api';

describe( 'BlockEdit', () => {
afterEach( () => {
getBlockTypes().forEach( ( block ) => {
unregisterBlockType( block.name );
} );
} );

it( 'should return null if block type not defined', () => {
const wrapper = shallow( <BlockEdit name="core/test-block" /> );

expect( wrapper.type() ).toBe( null );
} );

it( 'should use edit implementation of block', () => {
const edit = () => <div />;
registerBlockType( 'core/test-block', {
save: noop,
category: 'common',
title: 'block title',
edit,
} );

const wrapper = shallow( <BlockEdit name="core/test-block" /> );

expect( wrapper.type() ).toBe( edit );
} );

it( 'should use save implementation of block as fallback', () => {
const save = () => <div />;
registerBlockType( 'core/test-block', {
save,
category: 'common',
title: 'block title',
} );

const wrapper = shallow( <BlockEdit name="core/test-block" /> );

expect( wrapper.type() ).toBe( save );
} );
} );
Loading