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

Block API: Adding the anchor support (id) #2394

Merged
merged 10 commits into from
Aug 18, 2017
Merged

Block API: Adding the anchor support (id) #2394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dfc676b
Block API: Adding the anchor support (id)
youknowriad Aug 14, 2017
4e78521
Blocks: Adding a help text to the anchor input
youknowriad Aug 14, 2017
5474dae
Blocks: Update the anchor regex to allow special chars except spaces …
youknowriad Aug 15, 2017
21fbadd
Inspector Controls: Adding help support to all inspector controls
youknowriad Aug 15, 2017
c7d550f
Blocks: Rename the generic controls (id, class) to advanced controls
youknowriad Aug 15, 2017
bb4173d
Advanced Controls: Show the permalink with anchor and copy link button
youknowriad Aug 17, 2017
66f9fdf
Advanced controls: Fix Parsing Blocks with anchors
youknowriad Aug 17, 2017
0306e4e
Advanced controls: Fix the condition for hiding the classname control
youknowriad Aug 17, 2017
39efa0d
Advanced Controls: Shorten the displayed permalink
youknowriad Aug 17, 2017
9cbd343
Move permalink with anchor to a tooltip.
mtias Aug 17, 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
15 changes: 9 additions & 6 deletions blocks/api/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,18 @@ import { getBlockType } from './registration';
/**
* Returns a block object given its type and attributes.
*
* @param {String} name Block name
* @param {Object} attributes Block attributes
* @return {Object} Block object
* @param {String} name Block name
* @param {Object} blockAttributes Block attributes
* @return {Object} Block object
*/
export function createBlock( name, attributes = {} ) {
export function createBlock( name, blockAttributes = {} ) {
// Get the type definition associated with a registered block.
const blockType = getBlockType( name );

// Ensure attributes contains only values defined by block type, and merge
// default values for missing attributes.
attributes = reduce( blockType.attributes, ( result, source, key ) => {
const value = attributes[ key ];
const attributes = reduce( blockType.attributes, ( result, source, key ) => {
const value = blockAttributes[ key ];
if ( undefined !== value ) {
result[ key ] = value;
} else if ( source.default ) {
Expand All @@ -39,6 +39,9 @@ export function createBlock( name, attributes = {} ) {

return result;
}, {} );
if ( blockType.supportAnchor && blockAttributes.anchor ) {
attributes.anchor = blockAttributes.anchor;
}

// Blocks are stored with a unique ID, the assigned type name,
// and the block attributes.
Expand Down
11 changes: 9 additions & 2 deletions blocks/api/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { parse as hpqParse } from 'hpq';
import { parse as hpqParse, attr } from 'hpq';
import { mapValues, reduce, pickBy } from 'lodash';

/**
Expand Down Expand Up @@ -101,7 +101,7 @@ export function getBlockAttributes( blockType, rawContent, attributes ) {
blockType.attributes
);

return reduce( blockType.attributes, ( result, source, key ) => {
const blockAttributes = reduce( blockType.attributes, ( result, source, key ) => {
let value;
if ( sourcedAttributes.hasOwnProperty( key ) ) {
value = sourcedAttributes[ key ];
Expand Down Expand Up @@ -146,6 +146,13 @@ export function getBlockAttributes( blockType, rawContent, attributes ) {
result[ key ] = coercedValue;
return result;
}, {} );

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

return blockAttributes;
}

/**
Expand Down
26 changes: 17 additions & 9 deletions blocks/api/serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,28 @@ export function getSaveContent( blockType, attributes ) {
}

// Adding a generic classname
const addClassnameToElement = ( element ) => {
if ( ! element || ! isObject( element ) || ! className ) {
const addAdvancedAttributes = ( element ) => {
if ( ! element || ! isObject( element ) ) {
return element;
}

const updatedClassName = classnames(
className,
element.props.className,
attributes.className
);
const extraProps = {};
if ( !! className ) {
const updatedClassName = classnames(
className,
element.props.className,
attributes.className
);
extraProps.className = updatedClassName;
}

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

return cloneElement( element, { className: updatedClassName } );
return cloneElement( element, extraProps );
};
const contentWithClassname = Children.map( rawContent, addClassnameToElement );
const contentWithClassname = Children.map( rawContent, addAdvancedAttributes );

// Otherwise, infer as element
return renderToString( contentWithClassname );
Expand Down
23 changes: 23 additions & 0 deletions blocks/api/test/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,29 @@ describe( 'block factory', () => {
expect( block.isValid ).toBe( true );
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',
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 );
} );
} );

describe( 'switchToBlockType()', () => {
Expand Down
20 changes: 20 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 @@ -155,6 +155,26 @@ describe( 'block parser', () => {
topic: 'none',
} );
} );

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

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

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

describe( 'createBlockWithFallback', () => {
Expand Down
14 changes: 14 additions & 0 deletions blocks/api/test/serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,20 @@ 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
3 changes: 2 additions & 1 deletion blocks/inspector-controls/base-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@ import classnames from 'classnames';
*/
import './style.scss';

function BaseControl( { id, label, className, children } ) {
function BaseControl( { id, label, help, className, children } ) {
return (
<div className={ classnames( 'blocks-base-control', className ) }>
{ label && <label className="blocks-base-control__label" htmlFor={ id }>{ label }</label> }
{ children }
{ !! help && <p id={ id + '__help' } className="blocks-base-control__help">{ help }</p> }
</div>
);
}
Expand Down
4 changes: 4 additions & 0 deletions blocks/inspector-controls/base-control/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@
display: block;
margin-bottom: 5px;
}

.blocks-base-control__help {
font-style: italic;
}
5 changes: 3 additions & 2 deletions blocks/inspector-controls/checkbox-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,20 @@ import { withInstanceId } from '@wordpress/components';
import BaseControl from './../base-control';
import './style.scss';

function CheckboxControl( { label, heading, checked, instanceId, onChange, ...props } ) {
function CheckboxControl( { label, heading, checked, help, instanceId, onChange, ...props } ) {
const id = 'inspector-checkbox-control-' + instanceId;
const onChangeValue = ( event ) => onChange( event.target.value );

return (
<BaseControl label={ heading } id={ id }>
<BaseControl label={ heading } id={ id } help={ help }>
<input
id={ id }
className="blocks-checkbox-control__input"
type="checkbox"
value="1"
onChange={ onChangeValue }
checked={ checked }
aria-describedby={ !! help ? id + '__help' : undefined }
{ ...props }
/>
<label className="blocks-checkbox-control__label" htmlFor={ id }>
Expand Down
5 changes: 3 additions & 2 deletions blocks/inspector-controls/radio-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ import { withInstanceId } from '@wordpress/components';
import BaseControl from './../base-control';
import './style.scss';

function RadioControl( { label, selected, instanceId, onChange, options = [] } ) {
function RadioControl( { label, selected, help, instanceId, onChange, options = [] } ) {
const id = 'inspector-radio-control-' + instanceId;
const onChangeValue = ( event ) => onChange( event.target.value );

return ! isEmpty( options ) && (
<BaseControl label={ label } id={ id } className="blocks-radio-control">
<BaseControl label={ label } id={ id } help={ help } className="blocks-radio-control">
{ options.map( ( option, index ) =>
<div
key={ ( id + '-' + index ) }
Expand All @@ -33,6 +33,7 @@ function RadioControl( { label, selected, instanceId, onChange, options = [] } )
value={ option.value }
onChange={ onChangeValue }
selected={ option.value === selected }
aria-describedby={ !! help ? id + '__help' : undefined }
/>
<label key={ option.value } htmlFor={ ( id + '-' + index ) }>
{ option.label }
Expand Down
5 changes: 3 additions & 2 deletions blocks/inspector-controls/range-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,20 @@ import { Dashicon, withInstanceId } from '@wordpress/components';
import BaseControl from './../base-control';
import './style.scss';

function RangeControl( { label, value, instanceId, onChange, beforeIcon, afterIcon, ...props } ) {
function RangeControl( { label, value, instanceId, onChange, beforeIcon, afterIcon, help, ...props } ) {
const id = 'inspector-range-control-' + instanceId;
const onChangeValue = ( event ) => onChange( Number( event.target.value ) );

return (
<BaseControl label={ label } id={ id } className="blocks-range-control">
<BaseControl label={ label } id={ id } help={ help } className="blocks-range-control">
{ beforeIcon && <Dashicon icon={ beforeIcon } size={ 20 } /> }
<input
className="blocks-range-control__slider"
id={ id }
type="range"
value={ value }
onChange={ onChangeValue }
aria-describedby={ !! help ? id + '__help' : undefined }
{ ...props } />
{ afterIcon && <Dashicon icon={ afterIcon } /> }
<input
Expand Down
5 changes: 3 additions & 2 deletions blocks/inspector-controls/select-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,17 @@ import { withInstanceId } from '@wordpress/components';
import BaseControl from './../base-control';
import './style.scss';

function SelectControl( { label, selected, instanceId, onBlur, options = [], ...props } ) {
function SelectControl( { label, selected, help, instanceId, onBlur, options = [], ...props } ) {
const id = 'inspector-select-control-' + instanceId;
const onBlurValue = ( event ) => onBlur( event.target.value );

return ! isEmpty( options ) && (
<BaseControl label={ label } id={ id }>
<BaseControl label={ label } id={ id } help={ help }>
<select
id={ id }
className="blocks-select-control__input"
onBlur={ onBlurValue }
aria-describedby={ !! help ? id + '__help' : undefined }
{ ...props }
>
{ options.map( ( option ) =>
Expand Down
13 changes: 10 additions & 3 deletions blocks/inspector-controls/text-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,20 @@ import { withInstanceId } from '@wordpress/components';
import BaseControl from './../base-control';
import './style.scss';

function TextControl( { label, value, instanceId, onChange, type = 'text', ...props } ) {
function TextControl( { label, value, help, instanceId, onChange, type = 'text', ...props } ) {
const id = 'inspector-text-control-' + instanceId;
const onChangeValue = ( event ) => onChange( event.target.value );

return (
<BaseControl label={ label } id={ id }>
<input className="blocks-text-control__input" type={ type } id={ id } value={ value } onChange={ onChangeValue } { ...props } />
<BaseControl label={ label } id={ id } help={ help }>
<input className="blocks-text-control__input"
type={ type }
id={ id }
value={ value }
onChange={ onChangeValue }
aria-describedby={ !! help ? id + '__help' : undefined }
{ ...props }
/>
</BaseControl>
);
}
Expand Down
13 changes: 10 additions & 3 deletions blocks/inspector-controls/textarea-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,20 @@ import { withInstanceId } from '@wordpress/components';
import BaseControl from './../base-control';
import './style.scss';

function TextareaControl( { label, value, instanceId, onChange, rows = 4, ...props } ) {
function TextareaControl( { label, value, help, instanceId, onChange, rows = 4, ...props } ) {
const id = 'inspector-textarea-control-' + instanceId;
const onChangeValue = ( event ) => onChange( event.target.value );

return (
<BaseControl label={ label } id={ id }>
<textarea className="blocks-textarea-control__input" id={ id } rows={ rows } onChange={ onChangeValue } { ...props }>
<BaseControl label={ label } id={ id } help={ help }>
<textarea
className="blocks-textarea-control__input"
id={ id }
rows={ rows }
onChange={ onChangeValue }
aria-describedby={ !! help ? id + '__help' : undefined }
{ ...props }
>
{ value }
</textarea>
</BaseControl>
Expand Down
11 changes: 8 additions & 3 deletions blocks/inspector-controls/toggle-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,17 @@ import { withInstanceId, FormToggle } from '@wordpress/components';
import BaseControl from './../base-control';
import './style.scss';

function ToggleControl( { label, checked, instanceId, onChange } ) {
function ToggleControl( { label, checked, help, instanceId, onChange } ) {
const id = 'inspector-toggle-control-' + instanceId;

return (
<BaseControl label={ label } id={ id } className="blocks-toggle-control">
<FormToggle id={ id } checked={ checked } onChange={ onChange } />
<BaseControl label={ label } id={ id } help={ help } className="blocks-toggle-control">
<FormToggle
id={ id }
checked={ checked }
onChange={ onChange }
aria-describedby={ !! help ? id + '__help' : undefined }
/>
</BaseControl>
);
}
Expand Down
2 changes: 2 additions & 0 deletions blocks/library/heading/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ registerBlockType( 'core/heading', {

className: false,

supportAnchor: true,
Copy link
Member

Choose a reason for hiding this comment

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

Trying to think of another way we might scale this. A similar pattern exists in core with post type supports and theme supports. It's a bit different in that case because those supports are always opt-in, which could be represented as an array. If we need some default-true supports, maybe it could still be defined as an object?

supports: {
	className: false,
	anchor: true,
},

And we could then have a utility method for getting block supports accounting for the default.

Are there other "supports" types we could see adding in the future?

Should className be opt-in? Then we could have:

supports: [
	'className',
	'anchor',
],

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 personally think the block alignments should be part of these generic attributes. And I don't have a strong preference on how to declare support for these attributes.

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 terribly happy with the way className works currently, and I'm curious if it might be improved by changing it to an opt-in. Relating to #2035, it's just not very obvious how this works, particularly when implementing a block with a string return value from save. Changing this to an opt-in at least signals that the block implementer has some understanding of how it works and how they might need to implement the behavior on their own. Otherwise, they can fall into the trap of implementing a string return from save, and users of their block will be presented with a non-functional class name field.

As it relates here, this would have the side-benefit of granting us a consistent pattern for supports. Either it's undeclared and unsupported, or it's declared and therefore supported. Further, this is consistent with how supports work with custom post types and themes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this sounds like a good direction to me. I think @mtias wanted it to be opt-out at first?

Do you think I should update this PR or should we make this consistent separately?

Copy link
Member

Choose a reason for hiding this comment

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

There was another related discussion previously, I think when className was introduced, about the proliferation/scalability of properties on the block API. It may have been @BE-Webdesign that raised it? In any case, it seems wise to build a pattern around such "boolean" features (either block supports it or it doesn't). Even the prefix as it stands here is indicative of scaling trouble: supportAnchor, eventually we might need supportFoo, supportBar.

I don't mean for it to cause trouble for this pull request specifically, but if it's something we feel we ought to address, I think it should either occur here or prior to this pull request being merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm, one thing that come to my mind, is that the "className" attribute is not only to declare className support but it serves as a fallback className if it's a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think when className was introduced, about the proliferation/scalability of properties on the block API. It may have been @BE-Webdesign that raised it?

Yeah that was me and it was around useOnce/useMany. That property especially might cause problems as it could have an entirely different value in different contexts.

A supports object sounds like a reasonable way forward. Haven't checked out the branch at all to see how it works.

Copy link
Member

Choose a reason for hiding this comment

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

Mmm, one thing that come to my mind, is that the "className" attribute is not only to declare className support but it serves as a fallback className if it's a string.

What requirements is this feature serving? (Why do we need 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.

It was just an easy way to override the default generated className if a plugin author wanted to do so. But we could remove it for now.

Copy link
Member

Choose a reason for hiding this comment

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

It was just an easy way to override the default generated className if a plugin author wanted to do so. But we could remove it for now.

Can't a block author do this themselves by assigning className: false then assigning a class name of their choosing to the returned element?


attributes: {
content: {
type: 'array',
Expand Down
3 changes: 2 additions & 1 deletion components/form-toggle/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { __ } from '@wordpress/i18n';
*/
import './style.scss';

function FormToggle( { className, checked, id, onChange = noop, showHint = true } ) {
function FormToggle( { className, checked, id, onChange = noop, showHint = true, ...props } ) {
const wrapperClasses = classnames(
'components-form-toggle',
className,
Expand All @@ -29,6 +29,7 @@ function FormToggle( { className, checked, id, onChange = noop, showHint = true
type="checkbox"
checked={ checked }
onChange={ onChange }
{ ...props }
/>
<span className="components-form-toggle__track"></span>
<span className="components-form-toggle__thumb"></span>
Expand Down
Loading