Skip to content

Blocks: Handle pressing backspace at the beginning of blocks #461

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

Merged
youknowriad merged 9 commits into from
Apr 27, 2017
Merged

Blocks: Handle pressing backspace at the beginning of blocks #461

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5e4945d
Blocks: Handle the backspace at the beginning of blocks
youknowriad Apr 20, 2017
7106128
replacing `childNodes[ 0 ]` with `firstChild`
youknowriad Apr 24, 2017
d5fc22a
Element: Rename concatChildren helper and add unit tests
youknowriad Apr 25, 2017
f0c34ba
Style: avoid ternary and fallback using ||
youknowriad Apr 25, 2017
ae4f41a
Fix merging two consecutive empty headings
youknowriad Apr 26, 2017
15b70c1
Fix TinyMCE isCollapsed error when merging two blocks
youknowriad Apr 26, 2017
129ffd7
Stop keydown event propagation if editor removed by merge
aduth Apr 26, 2017
1b360dc
Update concatChildren to accomodate multiple children
youknowriad Apr 27, 2017
a2c1065
ConcatChildren: Fix unit tests
youknowriad Apr 27, 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
Prev Previous commit
Next Next commit
Element: Rename concatChildren helper and add unit tests
  • Loading branch information
@youknowriad
youknowriad committed Apr 27, 2017
commit d5fc22a190499088010baab58228116228c6f25f
2 changes: 1 addition & 1 deletion blocks/library/heading/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ registerBlock( 'core/heading', {

merge( attributes, attributesToMerge ) {
return {
content: wp.element.concatValues( attributes.content, attributesToMerge.content )
content: wp.element.concatChildren( attributes.content, attributesToMerge.content )
};
},

Expand Down
2 changes: 1 addition & 1 deletion blocks/library/text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ registerBlock( 'core/text', {

merge( attributes, attributesToMerge ) {
return {
content: wp.element.concatValues( attributes.content, attributesToMerge.content )
content: wp.element.concatChildren( attributes.content, attributesToMerge.content )
};
},

Expand Down
41 changes: 28 additions & 13 deletions element/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,19 +67,34 @@ export function renderToString( element ) {
return renderToStaticMarkup( element );
}

export function concatValues( value1, value2 ) {
const toArray = value => Array.isArray( value ) ? Children.toArray( value ) : [ value ];
/**
* Concat two React children objects
*
* @param {?Object} children1 First children value
* @param {?Object} children2 Second children value
*
* @return {Array} The concatenation
*/
export function concatChildren( children1, children2 ) {
Copy link
Member

@aduth aduth Apr 26, 2017

Choose a reason for hiding this comment

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

I was poking around at this function, and had a couple observations:

  • I think it'd be easy enough and a nice feature to support a spread value of ...childrens, so two or more set of children can be concatenated
  • I'm still unclear why toArray need exist. You'd mentioned different values, but in my testing Children.toArray seems to handle most everything thrown at it.
  • I think we might be able to drop an iteration by creating the return value while iterating children (instead of toArray then map)

Do you think something like this would work?

/**
 * Concatenate two or more React children objects
 *
 * @param  {...?Object} childrens Set of children to concatenate
 * @return {Array}                The concatenated value
 */
export function concatChildren( ...childrens ) {
	return childrens.reduce( ( memo, children, i ) => {
		Children.forEach( children, ( child, l ) => {
			memo.push( cloneElement( child, {
				key: [ i, l ].join()
			} ) );
		} );

		return memo;
	}, [] );
}

Copy link
Contributor Author

@youknowriad youknowriad Apr 26, 2017

Choose a reason for hiding this comment

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

I think it'd be easy enough and a nice feature to support a spread value of ...childrens, so two or more set of children can be concatenated

Great idea!

I've tried your version but it's not working. I'm finding the toArray to be necessary. I'm unsure if it may be a problem coming from another place (parser or anything), but you can easily reproduce a bug by backspacing from the start of the second test block "I imagine..."

Copy link
Member

@aduth aduth Apr 26, 2017

Choose a reason for hiding this comment

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

Huh, yeah, I can reproduce that. Seems to be an issue at the point of calling cloneElement on the string though. This variation seems to work okay for me:

/**
 * Concatenate two or more React children objects
 *
 * @param  {...?Object} childrens Set of children to concatenate
 * @return {Array}                The concatenated value
 */
export function concatChildren( ...childrens ) {
	return childrens.reduce( ( memo, children, i ) => {
		Children.forEach( children, ( child, j ) => {
			if ( 'string' !== typeof child ) {
				child = cloneElement( child, {
					key: [ i, j ].join()
				} );
			}

			memo.push( child );
		} );

		return memo;
	}, [] );
}

const toArray = ( children ) => {
if ( ! children ) {
return [];
}

return Array.isArray( children ) ? Children.toArray( children ) : [ children ];
};

return toArray( value1 )
.concat( toArray( value2 ) )
.map( ( elt, index ) => {
if ( isString( elt ) ) {
return elt;
}
return [
...toArray( children1 ),
...toArray( children2 )
].map( ( elt, index ) => {
if ( isString( elt ) ) {
return elt;
}

return {
...elt,
key: index
};
} );
return {
...elt,
key: index
};
} );
}
Copy link
Member

@aduth aduth Apr 24, 2017

Choose a reason for hiding this comment

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

Evidenced by my confusion on the behavior of this function, would be good to add some JSDoc and test cases.

Copy link
Contributor Author

@youknowriad youknowriad Apr 24, 2017

Choose a reason for hiding this comment

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

Yep, and unit tests too :)

22 changes: 21 additions & 1 deletion element/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { expect } from 'chai';
/**
* Internal dependencies
*/
import { createElement, renderToString } from '../';
import { createElement, renderToString, concatChildren } from '../';

describe( 'element', () => {
describe( 'renderToString', () => {
Expand Down Expand Up @@ -35,4 +35,24 @@ describe( 'element', () => {
) ).to.equal( '<strong>Courgette</strong>' );
} );
} );

describe( 'concatChildren', () => {
it( 'should return an empty array for undefined children', () => {
expect( concatChildren() ).to.eql( [] );
} );

it( 'should concat the string arrays', () => {
expect( concatChildren( [ 'a' ], 'b' ) ).to.eql( [ 'a', 'b' ] );
} );

it( 'should concat the object arrays and rewrite keys', () => {
const concat = concatChildren(
[ createElement( 'strong', null, 'Courgette' ) ],
createElement( 'strong', null, 'Concombre' )
);
expect( concat.length ).to.equal( 2 );
expect( concat[ 0 ].key = 0 );
expect( concat[ 1 ].key = 1 );
} );
} );
} );