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

Extensibility: Define customClassname behavior as filtered blocks support #3472

Merged
merged 7 commits into from Nov 17, 2017
Merged

Extensibility: Define customClassname behavior as filtered blocks support #3472

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e1e8138
Extensibility: Define customClassname behavior as filtered blocks sup…
youknowriad Nov 14, 2017
6667db0
Extensibility: Fix extending the BlockEdit
youknowriad Nov 16, 2017
6d94b55
Extensibility: Changes per review for the Generated ClassName extension
youknowriad Nov 16, 2017
74b8cfa
Tests: Fix registration unit test post rebase
youknowriad Nov 16, 2017
3d54d33
Blocks: Try to fix rerender issue for custom class name
gziolo Nov 16, 2017
73c3e30
Components: Preserve fill ordering through unmounts
aduth Nov 16, 2017
5c38f6a
Extensibility: Unify className word usage
gziolo Nov 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions blocks/block-edit/index.js
View file
Open in desktop
Expand Up @@ -15,8 +15,10 @@ function BlockEdit( props ) {
// `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.
const Edit = blockType.edit || blockType.save;
Edit.displayName = 'Edit';
let Edit;
if ( blockType ) {
Copy link
Member

@gziolo gziolo Nov 16, 2017 •
edited

Choose a reason for hiding this comment

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

I changed that because there is the opposite check above:

if ( ! blockType ) {
    return null;
}

This check is obsolete. It will never be falsy here.

Edit = blockType.edit || blockType.save;
}

return applyFilters( 'BlockEdit', <Edit key="edit" { ...editProps } />, props );
}
Expand Down
9 changes: 4 additions & 5 deletions blocks/hooks/anchor.js
View file
Open in desktop
Expand Up @@ -6,7 +6,6 @@ import { assign } from 'lodash';
/**
* WordPress dependencies
*/
import { concatChildren } from '@wordpress/element';
import { __ } from '@wordpress/i18n';

/**
Expand Down Expand Up @@ -55,9 +54,9 @@ export function addAttribute( settings ) {
*/
export function addInspectorControl( element, props ) {
if ( hasBlockSupport( props.name, 'anchor' ) && props.focus ) {
element = concatChildren(
element = [
element,
<InspectorControls>
<InspectorControls key="inspector-anchor">
<InspectorControls.TextControl
label={ __( 'HTML Anchor' ) }
help={ __( 'Anchors lets you link directly to a section on a page.' ) }
Expand All @@ -69,8 +68,8 @@ export function addInspectorControl( element, props ) {
anchor: nextValue,
} );
} } />
</InspectorControls>
);
</InspectorControls>,
];
}

return element;
Expand Down
9 changes: 4 additions & 5 deletions blocks/hooks/custom-class-name.js
View file
Open in desktop
Expand Up @@ -7,7 +7,6 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { concatChildren } from '@wordpress/element';
import { __ } from '@wordpress/i18n';

/**
Expand Down Expand Up @@ -46,9 +45,9 @@ export function addAttribute( settings ) {
*/
export function addInspectorControl( element, props ) {
if ( hasBlockSupport( props.name, 'customClassName', true ) && props.focus ) {
element = concatChildren(
element = [
Copy link
Member

Choose a reason for hiding this comment

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

I tested it a bit, it works with concatChildren, too.

element,
<InspectorControls force>
<InspectorControls key="inspector-custom-classname">
<InspectorControls.TextControl
label={ __( 'Additional CSS Class' ) }
value={ props.attributes.className || '' }
Expand All @@ -58,8 +57,8 @@ export function addInspectorControl( element, props ) {
} );
} }
/>
</InspectorControls>
);
</InspectorControls>,
];
}

return element;
Expand Down
4 changes: 2 additions & 2 deletions blocks/inspector-controls/index.js
View file
Open in desktop
Expand Up @@ -15,9 +15,9 @@ import TextControl from './text-control';
import TextareaControl from './textarea-control';
import ToggleControl from './toggle-control';

export default function InspectorControls( { force = false, children } ) {
export default function InspectorControls( { children } ) {
return (
<Fill name="Inspector.Controls" force={ force }>
<Fill name="Inspector.Controls">
{ children }
</Fill>
);
Expand Down
25 changes: 18 additions & 7 deletions components/slot-fill/fill.js
View file
Open in desktop
Expand Up @@ -8,32 +8,39 @@ import { noop } from 'lodash';
*/
import { Component, createPortal } from '@wordpress/element';

/**
* Internal dependencies
*/
import withInstanceId from '../higher-order/with-instance-id';
let occurrences = 0;

class Fill extends Component {
componentWillMount() {
this.occurrence = ++occurrences;
}

componentDidMount() {
const { registerFill = noop } = this.context;

registerFill( this.props.name, this );
}

componentWillUpdate() {
if ( ! this.occurrence ) {
this.occurrence = ++occurrences;
}
}

componentWillUnmount() {
const { unregisterFill = noop } = this.context;

unregisterFill( this.props.name, this );
}

componentWillReceiveProps( nextProps ) {
const { name, force } = nextProps;
const { name } = nextProps;
const {
unregisterFill = noop,
registerFill = noop,
} = this.context;

if ( this.props.name !== name || force ) {
if ( this.props.name !== name ) {
unregisterFill( this.props.name, this );
registerFill( name, this );
}
Expand All @@ -47,6 +54,10 @@ class Fill extends Component {
}
}

resetOccurrence() {
this.occurrence = null;
}

render() {
const { getSlot = noop } = this.context;
const { name, children } = this.props;
Expand All @@ -65,4 +76,4 @@ Fill.contextTypes = {
unregisterFill: noop,
};

export default withInstanceId( Fill );
export default Fill;
19 changes: 12 additions & 7 deletions components/slot-fill/provider.js
View file
Open in desktop
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { pick, without, noop } from 'lodash';
import { pick, sortBy, forEach, without, noop } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -61,6 +61,7 @@ class SlotFillProvider extends Component {
this.fills[ name ],
instance
);
this.resetFillOccurrence( name );
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the most important line. It causes the key to update in slot implementation. 💯

this.forceUpdateSlot( name );
}

Expand All @@ -69,15 +70,19 @@ class SlotFillProvider extends Component {
}

getFills( name ) {
return this.fills[ name ];
return sortBy( this.fills[ name ], 'occurrence' );
}

resetFillOccurrence( name ) {
forEach( this.fills[ name ], ( instance ) => {
instance.resetOccurrence();
} );
}

forceUpdateFills( name ) {
if ( this.fills.hasOwnProperty( name ) ) {
this.fills[ name ].forEach( ( instance ) => {
instance.forceUpdate();
} );
}
forEach( this.fills[ name ], ( instance ) => {
instance.forceUpdate();
} );
}

forceUpdateSlot( name ) {
Expand Down
2 changes: 1 addition & 1 deletion components/slot-fill/slot.js
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@ class Slot extends Component {
return (
<div ref={ this.bindNode }>
{ map( getFills( name ), ( fill ) => {
const fillKey = fill.props.instanceId;
const fillKey = fill.occurrence;
return Children.map( fill.props.children, ( child, childIndex ) => {
if ( ! child || isString( child ) ) {
return child;
Expand Down
35 changes: 34 additions & 1 deletion components/slot-fill/test/slot.js
View file
Open in desktop
Expand Up @@ -26,7 +26,7 @@ class Filler extends Component {
render() {
return [
<button key="1" type="button" onClick={ () => this.setState( { num: this.state.num + 1 } ) } />,
<Fill name={ this.props.name } key="2">{ this.state.num.toString() }</Fill>,
<Fill name={ this.props.name } key="2">{ this.props.text || this.state.num.toString() }</Fill>,
];
}
}
Expand Down Expand Up @@ -96,4 +96,37 @@ describe( 'Slot', () => {

expect( element.find( 'Slot > div' ).html() ).toBe( '<div>2</div>' );
} );

it( 'should render in expected order', () => {
const element = mount(
<Provider>
<Slot name="egg" key="slot" />
</Provider>
);

element.setProps( {
children: [
<Slot name="egg" key="slot" />,
<Filler name="egg" key="first" text="first" />,
<Filler name="egg" key="second" text="second" />,
],
} );

element.setProps( {
children: [
<Slot name="egg" key="slot" />,
<Filler name="egg" key="second" text="second" />,
],
} );

element.setProps( {
children: [
<Slot name="egg" key="slot" />,
<Filler name="egg" key="first" text="first" />,
<Filler name="egg" key="second" text="second" />,
],
} );
Copy link
Member

@gziolo gziolo Nov 16, 2017 •
edited

Choose a reason for hiding this comment

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

element.setProps( {
	children: [
		<Slot name="egg" key="slot" />,
 		<Filler name="egg" key="second" text="second" />,
 		<Filler name="egg" key="first" text="first" />,
 	],
 } )

I tried the following instead of line 122 and it outputs <div>secondfirst</div>. Which means you can use still use concatChildren, because it doesn't care about the keys passed. However it is predictable because it resets occurences for every slot whenever any fill gets removed.

If I add the same code after line 128, it still renders <div>firstsecond</div> because occurrences are not modified.


expect( element.find( 'Slot > div' ).html() ).toBe( '<div>firstsecond</div>' );
} );
} );