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

Editable: state as a cleaned up tree #3048

Closed
wants to merge 11 commits into from
Closed

Editable: state as a cleaned up tree #3048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2bfc296
WIP
ellatrix Sep 25, 2017
7d243fa
Editable: Add missing Editable.Value wrappers to the existing blocks
gziolo Oct 19, 2017
0662a94
Editable: Add unit tests to verify Editable.Value
gziolo Oct 19, 2017
2a0611e
Testing: Regenerate test fixtures for full post content checks
gziolo Oct 23, 2017
2a1989c
Testing: Fix failing tests caused by the node matcher
gziolo Oct 23, 2017
54d7631
Framework: Final touches to allow saving blocks state as a tree
gziolo Oct 24, 2017
e50084a
Fix eslint error
ellatrix Oct 30, 2017
7177a6d
Fix docs
ellatrix Oct 30, 2017
9dab041
Try without dom-react
ellatrix Oct 31, 2017
df23eea
WIP
ellatrix Nov 3, 2017
e9b7111
Editable: Use valueToElement to render stored tree representation
gziolo Nov 3, 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
17 changes: 8 additions & 9 deletions blocks/api/matchers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
/**
* WordPress dependencies
* External dependencies
*/
import { createElement } from '@wordpress/element';
export { attr, prop, html, text, query } from 'hpq';

/**
* External dependencies
* Internal dependencies
*/
import { nodeListToReact, nodeToReact } from 'dom-react';
export { attr, prop, html, text, query } from 'hpq';
import { createSimpleNode, createSimpleNodeList } from './simple-dom';

export const children = ( selector ) => {
export const children = ( selector, filter ) => {
return ( domNode ) => {
let match = domNode;

Expand All @@ -18,21 +17,21 @@ export const children = ( selector ) => {
}

if ( match ) {
return nodeListToReact( match.childNodes || [], createElement );
return createSimpleNodeList( match.childNodes || [], filter );
}

return [];
};
};

export const node = ( selector ) => {
export const node = ( selector, filter ) => {
return ( domNode ) => {
let match = domNode;

if ( selector ) {
match = domNode.querySelector( selector );
}

return nodeToReact( match, createElement );
return createSimpleNode( match, filter );
};
};
65 changes: 65 additions & 0 deletions blocks/api/simple-dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/**
* External dependencies
*/
import { forEach } from 'lodash';

export function createSimpleNode( node, filter = Array ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some JSDoc for these functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure it's a WIP.

if ( ! node ) {
return null;
}

if ( node.nodeType === 3 ) {
Copy link
Member

Choose a reason for hiding this comment

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

This should either have a comment explaining the magic number, or referencing the node type constant Node.TEXT_NODE directly:

https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType#Node_type_constants

I think previously these were avoided due to browser support but with increased browser support, we should be okay. I've confirmed the constants exist in IE11:

IE11

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 know we've done that everywhere else, sorry. It's just a quick POC.

return node.nodeValue;
}

if ( node.nodeType !== 1 ) {
return null;
}

return filter(
node.nodeName.toLowerCase(),
Array.from( node.attributes || [] ).reduce( ( acc, { name, value } ) => ( {
Copy link
Member

Choose a reason for hiding this comment

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

Micro-optimization: Use Lodash's reduce directly and skip Array.from:

reduce( node.attributes, ( acc, { name, value } ) => ( {

...acc,
Copy link
Member

Choose a reason for hiding this comment

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

Micro-optimization / readability: While this reads nicely, it also creates a shallow clone unnecessarily on each iteration, where we'd be fine to mutate the accumulator:

reduce( node.attributes, ( acc, { name, value } ) => ( {
	acc[ name ] = value;
	return acc;
} )

Copy link
Member

Choose a reason for hiding this comment

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

or use a forEach instead of reduce if we want to break the semantics of reduce which is normally immutable 😉

forEach( node.attributes, ( { name, value } ) => acc[ name ] = value );

[ name ]: value,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, could Lodash's toPlainObject work here?

toPlainObject( node.attributes )

} ), {} ),
...createSimpleNodeList( node.childNodes || [], filter )
Copy link
Member

Choose a reason for hiding this comment

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

When will childNodes be falsey?

);
}

export function createSimpleNodeList( nodeList, filter ) {
return Array.from( nodeList ).reduce( ( acc, node ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Micro-optimization / readability: Use Lodash's reduce directly and skip Array.from:

reduce( nodeList, ( acc, node ) => ( {

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, given the task performed, this could be a good place for flatMap:

flatMap( nodeList, ( node ) => {
  const child = createSimpleNode( node, filter );
  const shouldNest = …;
  return shouldNest ? [ child ] : child;
} );

this assumes that, when shouldNest, child is already an array, otherwise we'd have to return [ [ child ] ] to counter the flattening.

const child = createSimpleNode( node, filter );

if ( Array.isArray( child ) && typeof child[ 0 ] !== 'string' ) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the cases where this condition varies? Needs comment and unit tests.

acc.push( ...child );
} else if ( child ) {
acc.push( child );
}

return acc;
}, [] );
}

// Smarter application in the future?
export function applySimpleNodeList( tree, node ) {
Copy link
Member

Choose a reason for hiding this comment

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

I was initially confused on the naming here, though I think maybe this function could be more useful / understandable if it were to return a document fragment that could be used in Editable via removeChild + appendChild.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, wasn't sure what the best way is, but went for appending right away. I thought we night eventually decide to create HTML from the tree and then set that instead. The nice thing about this is that it's fairly easy to create, and in the future we could do some DOM riffing to avoid updates where we can. I actually wonder if we there's nothing from React we can use there.

while ( node.firstChild ) {
node.removeChild( node.firstChild );
}

forEach( tree, ( piece ) => {
if ( typeof piece === 'string' ) {
node.appendChild( document.createTextNode( piece ) );
} else {
const [ name, attributes, ...children ] = piece;
const element = document.createElement( name );

forEach( attributes, ( value, key ) => {
element.setAttribute( key, value );
} );

applySimpleNodeList( children, element );

node.appendChild( element );
}
} );
}
25 changes: 16 additions & 9 deletions blocks/api/test/matchers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,40 +11,47 @@ import { renderToString } from '@wordpress/element';
/**
* Internal dependencies
*/
import * as sources from '../matchers';
import Editable from '../../editable';
import * as matchers from '../matchers';

describe( 'matchers', () => {
const html = '<blockquote><p>A delicious <b>sundae</b> dessert.</p><p>I want it!</p><footer>The Cook</footer></blockquote>';

describe( 'children()', () => {
it( 'should return a source function', () => {
const source = sources.children();
const source = matchers.children();

expect( typeof source ).toBe( 'function' );
} );

it( 'should return HTML equivalent WPElement of matched element', () => {
// Assumption here is that we can cleanly convert back and forth
// between a string and WPElement representation
const html = '<blockquote><p>A delicious sundae dessert</p></blockquote>';
const match = parse( html, sources.children() );
const match = parse( html, matchers.children() );

expect( renderToString( match ) ).toBe( html );
expect(
renderToString( <Editable.Value value={ match } /> )
).toBe( html );
} );
} );

describe( 'node()', () => {
it( 'should return a source function', () => {
const source = sources.node();
const source = matchers.node();

expect( typeof source ).toBe( 'function' );
} );

it( 'should return HTML equivalent WPElement of matched element', () => {
// Assumption here is that we can cleanly convert back and forth
// between a string and WPElement representation
const html = '<blockquote><p>A delicious sundae dessert</p></blockquote>';
const match = parse( html, sources.node() );
const match = parse( html, matchers.node() );

expect( renderToString( match ) ).toBe( `<body>${ html }</body>` );
expect(
renderToString( <Editable.Value value={ [ match ] } /> )
).toBe(
`<body>${ html }</body>`
);
} );
} );
} );
78 changes: 56 additions & 22 deletions blocks/editable/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,12 @@ import {
defer,
noop,
} from 'lodash';
import { nodeListToReact } from 'dom-react';
import 'element-closest';

/**
* WordPress dependencies
*/
import { createElement, Component, renderToString } from '@wordpress/element';
import { createElement, Component } from '@wordpress/element';
import { keycodes, createBlobURL } from '@wordpress/utils';
import { Slot, Fill } from '@wordpress/components';

Expand All @@ -29,6 +28,8 @@ import { Slot, Fill } from '@wordpress/components';
*/
import './style.scss';
import { rawHandler } from '../api';
import * as matchers from '../api/matchers';
import { applySimpleNodeList } from '../api/simple-dom';
import FormatToolbar from './format-toolbar';
import TinyMCE from './tinymce';
import { pickAriaProps } from './aria';
Expand All @@ -37,7 +38,10 @@ import { EVENTS } from './constants';

const { BACKSPACE, DELETE, ENTER } = keycodes;

function createTinyMCEElement( type, props, ...children ) {
const createNode = matchers.node( null, nodeFilter );
const createChildren = matchers.children( null, nodeFilter );

function nodeFilter( type, props, ...children ) {
if ( props[ 'data-mce-bogus' ] === 'all' ) {
return null;
}
Expand All @@ -46,11 +50,11 @@ function createTinyMCEElement( type, props, ...children ) {
return children;
}

return createElement(
return [
type,
omitBy( props, ( value, key ) => key.indexOf( 'data-mce-' ) === 0 ),
...children
);
omitBy( props, ( value, key ) => key.indexOf( 'data-mce-' ) === 0 || key === 'key' ),
...children,
];
}

function isLinkBoundary( fragment ) {
Expand All @@ -72,6 +76,39 @@ function getFormatProperties( formatName, parents ) {

const DEFAULT_FORMATS = [ 'bold', 'italic', 'strikethrough', 'link' ];

/**
* Transforms internal block's representation into an Element.
*
* @param {Array} value Value to transform
* @return {WPElement} Element.
*/
function valueToElement( value ) {
if ( ! Array.isArray( value ) ) {
return value;
}

return value.map( ( element, i ) => {
if ( typeof element === 'string' ) {
return element;
}

const [ type, props, ...children ] = element;
const reactProps = Object.keys( props ).reduce( ( accumulator, key ) => {
const mapping = {
class: 'className',
Copy link
Member

Choose a reason for hiding this comment

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

It looks like adding those 2 mappings is enough to satisfy React.

Copy link
Member

Choose a reason for hiding this comment

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

Okey, style needs its own handling, because I see this: https://reactjs.org/docs/error-decoder.html?invariant=62&args[]=

Copy link
Member

Choose a reason for hiding this comment

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

All keys must go camel case, too.

for: 'htmlFor',
};

return {
...accumulator,
[ mapping[ key ] ? mapping[ key ] : key ]: props[ key ],
};
}, { key: i } );

return createElement( type, { ...reactProps }, ...valueToElement( children ) );
Copy link
Member

Choose a reason for hiding this comment

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

It can be simply reactProps :)

} );
}

export default class Editable extends Component {
constructor( props ) {
super( ...arguments );
Expand Down Expand Up @@ -547,8 +584,8 @@ export default class Editable extends Component {
const index = dom.nodeIndex( selectedNode );
const beforeNodes = childNodes.slice( 0, index );
const afterNodes = childNodes.slice( index + 1 );
const beforeElement = nodeListToReact( beforeNodes, createTinyMCEElement );
const afterElement = nodeListToReact( afterNodes, createTinyMCEElement );
const beforeElement = beforeNodes.map( createNode );
const afterElement = afterNodes.map( createNode );

this.setContent( beforeElement );
this.props.onSplit( beforeElement, afterElement );
Expand Down Expand Up @@ -602,8 +639,8 @@ export default class Editable extends Component {
const beforeFragment = beforeRange.extractContents();
const afterFragment = afterRange.extractContents();

const beforeElement = nodeListToReact( beforeFragment.childNodes, createTinyMCEElement );
const afterElement = isLinkBoundary( afterFragment ) ? [] : nodeListToReact( afterFragment.childNodes, createTinyMCEElement );
const beforeElement = createChildren( beforeFragment );
const afterElement = isLinkBoundary( afterFragment ) ? [] : createChildren( afterFragment );

this.setContent( beforeElement );
this.props.onSplit( beforeElement, afterElement, ...blocks );
Expand Down Expand Up @@ -656,8 +693,8 @@ export default class Editable extends Component {
this.setContent( this.props.value );

this.props.onSplit(
nodeListToReact( before, createTinyMCEElement ),
nodeListToReact( after, createTinyMCEElement )
before.map( createNode ),
after.map( createNode )
);
}

Expand Down Expand Up @@ -687,17 +724,12 @@ export default class Editable extends Component {
this.editor.save();
}

setContent( content ) {
if ( ! content ) {
content = '';
}

content = renderToString( content );
this.editor.setContent( content, { format: 'raw' } );
setContent( content = [] ) {
applySimpleNodeList( content, this.editor.getBody() );
}

getContent() {
return nodeListToReact( this.editor.getBody().childNodes || [], createTinyMCEElement );
return createChildren( this.editor.getBody() );
}

updateFocus() {
Expand Down Expand Up @@ -850,7 +882,7 @@ export default class Editable extends Component {
getSettings={ this.getSettings }
onSetup={ this.onSetup }
style={ style }
defaultValue={ value }
defaultValue={ valueToElement( value ) }
Copy link
Member

Choose a reason for hiding this comment

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

Can we use applySimpleNodeList in here, too?

isPlaceholderVisible={ isPlaceholderVisible }
aria-label={ placeholder }
{ ...ariaProps }
Expand Down Expand Up @@ -879,3 +911,5 @@ Editable.defaultProps = {
formattingControls: DEFAULT_FORMATS,
formatters: [],
};

Editable.Value = ( { value } ) => valueToElement( value );
43 changes: 43 additions & 0 deletions blocks/editable/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,47 @@ describe( 'Editable', () => {
} );
} );
} );

describe( 'Editable.Value', () => {
const MyComponent = ( { value } ) => (
<div>
<Editable.Value value={ value } />
</div>
);

it( 'should render value containing string', () => {
const value = [ 'Hello, Dolly!' ];
const wrapper = shallow( <MyComponent value={ value } /> );

expect( wrapper.html() ).toBe( '<div>Hello, Dolly!</div>' );
} );

it( 'should render value containing a single DOM node', () => {
const value = [
[ 'h1', { class: 'my-class' }, 'This is a header' ],
];
const wrapper = shallow( <MyComponent value={ value } /> );

expect( wrapper.html() ).toBe( '<div><h1 class="my-class">This is a header</h1></div>' );
} );

it( 'should render value with deeply nested DOM nodes', () => {
const value = [
'This is a ',
[ 'strong', {}, 'paragraph' ],
' with a ',
[ 'a', { href: 'https://w.org/' }, 'link with ', [
'b', {}, 'bold ', [
'i', {}, 'and italics',
],
] ],
'.',
];
const wrapper = shallow( <MyComponent value={ value } /> );

expect( wrapper.html() ).toBe(
'<div>This is a <strong>paragraph</strong> with a <a href=\"https://w.org/\">link with <b>bold <i>and italics</i></b></a>.</div>'
);
} );
} );
} );
2 changes: 1 addition & 1 deletion blocks/library/audio/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ registerBlockType( 'core/audio', {
return (
<figure className={ align ? `align${ align }` : null }>
<audio controls="controls" src={ src } />
{ caption && caption.length > 0 && <figcaption>{ caption }</figcaption> }
{ caption && caption.length > 0 && <figcaption><Editable.Value value={ caption } /></figcaption> }
</figure>
);
},
Expand Down
2 changes: 1 addition & 1 deletion blocks/library/button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ registerBlockType( 'core/button', {
return (
<div className={ `align${ align }` } style={ { backgroundColor: color } }>
<a href={ url } title={ title } style={ { color: textColor } }>
{ text }
<Editable.Value value={ text } />
</a>
</div>
);
Expand Down
Loading