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

Add quote↔text block switching; add block switcher validation #465

Closed
wants to merge 8 commits into from
Closed

Add quote↔text block switching; add block switcher validation #465

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1aed21a
Move heading block `transforms` to before `edit` and `save`
nylen Apr 20, 2017
98a87ea
Disable switching a multi-paragraph text block to a heading block
nylen Apr 20, 2017
0b44dcd
Add transformations between quote and text blocks
nylen Apr 20, 2017
decdfcb
Fix lint errors
nylen Apr 20, 2017
c30cd92
Add a test for the new functionality
nylen Apr 20, 2017
d79463b
Rename quote block 'value' to 'content' (this matches the text block)
nylen Apr 20, 2017
854f1bb
Fix 'from' transformations
nylen Apr 20, 2017
a411591
When switching quote to text, include the citation as a paragraph
nylen Apr 20, 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
12 changes: 9 additions & 3 deletions blocks/api/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export function createBlock( blockType, attributes = {} ) {
*
* @param {Object} block Block object
* @param {string} blockType BlockType
* @return {Object?} Block object
* @return {?Object|Error} Block object, null, or error with failure message
*/
export function switchToBlockType( block, blockType ) {
// Find the right transformation by giving priority to the "to" transformation
Expand All @@ -45,9 +45,15 @@ export function switchToBlockType( block, blockType ) {
return null;
}

const attributes = transformation.transform( block.attributes );
if ( attributes instanceof Error ) {
// Blocks can perform validation and cancel transformations if needed.
return attributes;
Copy link
Contributor

@youknowriad youknowriad Apr 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Returning null above is also an error (no transformation found), we should be consistent.

In the same time, I'm wondering if we really need to throw specific errors. I don't see any use case where we'd want to perform different actions depending on the error. Should we show this error somewhere in the UI? I don't think so, it's not relevant for the user I think.

Note that the transforms API is not used for block switching only. If we introduce new returns values like these, we'd want to handle them in all "clients" see #457

Also, I think we should perform the transformation even if we have data loss (we should limit the data loss) because the user explicitly asks for the transformation, so it's not a surprise for him to get its content changed a bit. For example when switching from a text block to a heading block, I'm not agains concatening with spaces the different paragraphs. If we reject the transformation, this means backspacing from a text block to a heading won't work either.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would you suggest returning instead of null? To me this makes sense because it is a generic failure case which doesn't have specific meaning to the user (something went wrong in the code).

I'll address the rest of your comments below.

}

return Object.assign( {
uid: block.uid,
attributes: transformation.transform( block.attributes ),
blockType
attributes,
blockType,
} );
}
27 changes: 26 additions & 1 deletion blocks/api/test/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe( 'block factory', () => {
} );

describe( 'switchBlockType()', () => {
it( 'should switch the blockType of a block using the "transform form"', () => {
it( 'should switch the blockType of a block using the "transform from"', () => {
registerBlock( 'core/updated-text-block', {
transforms: {
from: [ {
Expand Down Expand Up @@ -116,5 +116,30 @@ describe( 'block factory', () => {

expect( updateBlock ).to.be.null();
} );

it( 'should allow blocks to abort a transformation', () => {
registerBlock( 'core/updated-text-block', {} );
registerBlock( 'core/text-block', {
transforms: {
to: [ {
blocks: [ 'core/updated-text-block' ],
transform: () => new Error( 'no soup for you' ),
} ]
}
} );

const block = {
uid: 1,
blockType: 'core/text-block',
attributes: {
value: 'ribs'
}
};

const updateBlock = switchToBlockType( block, 'core/updated-text-block' );

expect( updateBlock ).to.be.an.instanceof( Error );
expect( updateBlock.message ).to.eql( 'no soup for you' );
} );
} );
} );
67 changes: 35 additions & 32 deletions blocks/library/heading/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,47 +31,27 @@ registerBlock( 'core/heading', {
} ) )
],

edit( { attributes, setAttributes } ) {
const { content, tag, align } = attributes;

return (
<Editable
tagName={ tag }
value={ content }
onChange={ ( value ) => setAttributes( { content: value } ) }
style={ align ? { textAlign: align } : null }
/>
);
},

save( { attributes } ) {
const { align, tag: Tag, content } = attributes;

return (
<Tag
style={ align ? { textAlign: align } : null }
dangerouslySetInnerHTML={ { __html: content } } />
);
},

transforms: {
from: [
{
type: 'block',
blocks: [ 'core/text' ],
transform: ( { content, align } ) => {
if ( Array.isArray( content ) ) {
// TODO this appears to always be true?
// TODO reject the switch if more than one paragraph
if ( content.length > 1 ) {
return new Error(
'Block has more than one paragraph.'
);
}
content = content[ 0 ];
}
return {
tag: 'H2',
content,
align
align,
};
}
}
},
},
],
to: [
{
Expand All @@ -80,10 +60,33 @@ registerBlock( 'core/heading', {
transform: ( { content, align } ) => {
return {
content: [ content ],
align
align,
};
}
}
},
},
]
}
},

edit( { attributes, setAttributes } ) {
const { content, tag, align } = attributes;

return (
<Editable
tagName={ tag }
value={ content }
onChange={ ( value ) => setAttributes( { content: value } ) }
style={ align ? { textAlign: align } : null }
/>
);
},

save( { attributes } ) {
const { align, tag: Tag, content } = attributes;

return (
<Tag
style={ align ? { textAlign: align } : null }
dangerouslySetInnerHTML={ { __html: content } } />
);
},
} );
42 changes: 36 additions & 6 deletions blocks/library/quote/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,50 @@ registerBlock( 'core/quote', {
category: 'common',

attributes: {
value: query( 'blockquote > p', html() ),
content: query( 'blockquote > p', html() ),
citation: html( 'footer' )
},

transforms: {
from: [
{
type: 'block',
blocks: [ 'core/text' ],
transform: ( { content } ) => {
return {
content,
};
},
},
],
to: [
{
type: 'block',
blocks: [ 'core/text' ],
transform: ( { content, citation } ) => {
if ( citation ) {
return new Error(
'Quote citation would be lost on transform.'
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 we should just create two paragraphs instead of erroring here.

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 don't love this because switching a quote block to a text block and back would leave the citation as a separate paragraph.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a problem for me, because it's an explicit change asked by the user (maybe discuss in the comment bellow)

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 this is an acceptable data loss. I think the razor we should hold any transformation against is that it has to be non-destructive, which is fuzzy enough that it would okay this transformation, but disallow transforming it into a gallery for example (which would make no sense). In other words, yes you might lose some formatting, but the data is still there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I don't have a big problem with this specific case, I'll change it to include the citation as a separate paragraph.

);
}
return {
content,
};
},
},
],
},

edit( { attributes, setAttributes } ) {
const { value, citation } = attributes;
const { content, citation } = attributes;

return (
<blockquote className="blocks-quote">
<Editable
value={ fromValueToParagraphs( value ) }
value={ fromValueToParagraphs( content ) }
onChange={
( paragraphs ) => setAttributes( {
value: fromParagraphsToValue( paragraphs )
content: fromParagraphsToValue( paragraphs )
} )
} />
<footer>
Expand All @@ -46,11 +76,11 @@ registerBlock( 'core/quote', {
},

save( attributes ) {
const { value, citation } = attributes;
const { content, citation } = attributes;

return (
<blockquote>
{ value && value.map( ( paragraph, i ) => (
{ content && content.map( ( paragraph, i ) => (
<p
key={ i }
dangerouslySetInnerHTML={ {
Expand Down
67 changes: 47 additions & 20 deletions editor/components/block-switcher/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import { connect } from 'react-redux';
import { uniq, get, reduce } from 'lodash';
import { uniq, get, reduce, noop } from 'lodash';

/**
* Internal dependencies
Expand Down Expand Up @@ -35,18 +35,26 @@ class BlockSwitcher extends wp.element.Component {
}

render() {
const blockSettings = wp.blocks.getBlockSettings( this.props.block.blockType );
const blocksToBeTransformedFrom = reduce( wp.blocks.getBlocks(), ( memo, block ) => {
const transformFrom = get( block, 'transforms.from', [] );
const transformation = transformFrom.find( t => t.blocks.indexOf( this.props.block.blockType ) !== -1 );
return transformation ? memo.concat( [ block.slug ] ) : memo;
}, [] );
const { block } = this.props;

const blockSettings = wp.blocks.getBlockSettings( block.blockType );
const blocksToBeTransformedFrom = reduce(
wp.blocks.getBlocks(),
( memo, candidateBlock ) => {
const transformFrom = get( candidateBlock, 'transforms.from', [] );
const transformation = transformFrom.find(
t => t.blocks.indexOf( block.blockType ) !== -1
);
return transformation ? memo.concat( [ candidateBlock.slug ] ) : memo;
},
[]
);
const blocksToBeTransformedTo = get( blockSettings, 'transforms.to', [] )
.reduce( ( memo, transformation ) => memo.concat( transformation.blocks ), [] );
const allowedBlocks = uniq( blocksToBeTransformedFrom.concat( blocksToBeTransformedTo ) )
.reduce( ( memo, blockType ) => {
const block = wp.blocks.getBlockSettings( blockType );
return !! block ? memo.concat( block ) : memo;
const settings = wp.blocks.getBlockSettings( blockType );
return !! settings ? memo.concat( settings ) : memo;
}, [] );

if ( ! allowedBlocks.length ) {
Expand All @@ -65,21 +73,36 @@ class BlockSwitcher extends wp.element.Component {
{ this.state.open &&
<div className="editor-block-switcher__menu">
<div className="editor-block-switcher__menu-arrow" />
{ allowedBlocks.map( ( { slug, title, icon } ) => (
<IconButton
key={ slug }
onClick={ this.switchBlockType( slug ) }
className="editor-block-switcher__menu-item"
icon={ icon }
>
{ title }
</IconButton>
) ) }
{ allowedBlocks.map(
newBlock => this.renderAllowedBlock( block, newBlock )
) }
</div>
}
</div>
);
}

renderAllowedBlock( currentBlock, newBlockSettings ) {
const newBlockAttributes = wp.blocks.switchToBlockType(
currentBlock,
newBlockSettings.slug
);
const disabled = ( newBlockAttributes instanceof Error );
const disabledMessage = disabled ? newBlockAttributes.message : null;
const { slug, title, icon } = newBlockSettings;
return (
<IconButton
key={ slug }
onClick={ disabled ? noop : this.switchBlockType( slug ) }
className="editor-block-switcher__menu-item"
icon={ icon }
disabled={ disabled }
tooltip={ disabledMessage }
>
{ title }
</IconButton>
);
}
}

export default connect(
Expand All @@ -88,10 +111,14 @@ export default connect(
} ),
( dispatch, ownProps ) => ( {
onTransform( block, blockType ) {
block = wp.blocks.switchToBlockType( block, blockType );
if ( block instanceof Error ) {
return;
}
dispatch( {
type: 'SWITCH_BLOCK_TYPE',
uid: ownProps.uid,
block: wp.blocks.switchToBlockType( block, blockType )
block,
} );
}
} )
Expand Down
12 changes: 10 additions & 2 deletions editor/components/icon-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,19 @@ import './style.scss';
import Button from '../button';
import Dashicon from '../dashicon';

function IconButton( { icon, children, label, className, ...additionalProps } ) {
function IconButton( {
icon, children, label, className, tooltip,
...additionalProps
} ) {
const classes = classnames( 'editor-icon-button', className );

return (
<Button { ...additionalProps } aria-label={ label } className={ classes }>
<Button
{ ...additionalProps }
aria-label={ label }
className={ classes }
title={ tooltip }
>
<Dashicon icon={ icon } />
{ children }
</Button>
Expand Down