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.

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

Editor: Bring back Inspector Advanced Controls #5952

Merged
merged 5 commits into from
Apr 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
43 changes: 25 additions & 18 deletions blocks/hooks/anchor.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { assign } from 'lodash';
/**
* WordPress dependencies
*/
import { createHigherOrderComponent } from '@wordpress/element';
import { createHigherOrderComponent, Fragment } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';
import { TextControl } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
Expand All @@ -15,7 +15,7 @@ import { __ } from '@wordpress/i18n';
* Internal dependencies
*/
import { hasBlockSupport } from '../api';
import InspectorControls from '../inspector-controls';
import InspectorAdvancedControls from '../inspector-advanced-controls';

/**
* Regular expression matching invalid anchor characters for replacement.
Expand Down Expand Up @@ -58,22 +58,29 @@ export function addAttribute( settings ) {
*/
export const withInspectorControl = createHigherOrderComponent( ( BlockEdit ) => {
return ( props ) => {
const hasAnchor = hasBlockSupport( props.name, 'anchor' ) && props.isSelected;
return [
<BlockEdit key="block-edit-anchor" { ...props } />,
hasAnchor && <InspectorControls key="inspector-anchor">
<TextControl
label={ __( 'HTML Anchor' ) }
help={ __( 'Anchors lets you link directly to a section on a page.' ) }
value={ props.attributes.anchor || '' }
onChange={ ( nextValue ) => {
nextValue = nextValue.replace( ANCHOR_REGEX, '-' );
props.setAttributes( {
anchor: nextValue,
} );
} } />
</InspectorControls>,
];
const hasAnchor = hasBlockSupport( props.name, 'anchor' );

if ( hasAnchor && props.isSelected ) {
return (
<Fragment>
<BlockEdit { ...props } />
<InspectorAdvancedControls>
<TextControl
label={ __( 'HTML Anchor' ) }
help={ __( 'Anchors lets you link directly to a section on a page.' ) }
value={ props.attributes.anchor || '' }
onChange={ ( nextValue ) => {
nextValue = nextValue.replace( ANCHOR_REGEX, '-' );
props.setAttributes( {
anchor: nextValue,
} );
} } />
</InspectorAdvancedControls>
</Fragment>
);
}

return <BlockEdit { ...props } />;
};
}, 'withInspectorControl' );

Expand Down
40 changes: 23 additions & 17 deletions blocks/hooks/custom-class-name.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { createHigherOrderComponent } from '@wordpress/element';
import { createHigherOrderComponent, Fragment } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';
import { TextControl } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
Expand All @@ -16,7 +16,7 @@ import { __ } from '@wordpress/i18n';
* Internal dependencies
*/
import { hasBlockSupport } from '../api';
import InspectorControls from '../inspector-controls';
import InspectorAdvancedControls from '../inspector-advanced-controls';

/**
* Filters registered block settings, extending attributes with anchor using ID
Expand Down Expand Up @@ -49,22 +49,28 @@ export function addAttribute( settings ) {
*/
export const withInspectorControl = createHigherOrderComponent( ( BlockEdit ) => {
return ( props ) => {
const hasCustomClassName = hasBlockSupport( props.name, 'customClassName', true ) && props.isSelected;
const hasCustomClassName = hasBlockSupport( props.name, 'customClassName', true );

return [
<BlockEdit key="block-edit-custom-class-name" { ...props } />,
hasCustomClassName && <InspectorControls key="inspector-custom-class-name">
<TextControl
label={ __( 'Additional CSS Class' ) }
value={ props.attributes.className || '' }
onChange={ ( nextValue ) => {
props.setAttributes( {
className: nextValue,
} );
} }
/>
</InspectorControls>,
];
if ( hasCustomClassName && props.isSelected ) {
return (
<Fragment>
<BlockEdit { ...props } />
<InspectorAdvancedControls>
<TextControl
label={ __( 'Additional CSS Class' ) }
value={ props.attributes.className || '' }
onChange={ ( nextValue ) => {
props.setAttributes( {
className: nextValue,
} );
} }
/>
</InspectorAdvancedControls>
</Fragment>
);
}

return <BlockEdit { ...props } />;
};
}, 'withInspectorControl' );

Expand Down
1 change: 1 addition & 0 deletions blocks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export { default as Editable } from './rich-text/editable';
export { default as ImagePlaceholder } from './image-placeholder';
export { default as InnerBlocks } from './inner-blocks';
export { default as InspectorControls } from './inspector-controls';
export { default as InspectorAdvancedControls } from './inspector-advanced-controls';
export { default as PlainText } from './plain-text';
export { default as MediaUpload } from './media-upload';
export { default as RichText } from './rich-text';
Expand Down
6 changes: 6 additions & 0 deletions blocks/inspector-advanced-controls/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* WordPress dependencies
*/
import { createSlotFill } from '@wordpress/components';

export default createSlotFill( 'InspectorAdvancedControls' );
10 changes: 2 additions & 8 deletions blocks/inspector-controls/index.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
/**
* WordPress dependencies
*/
import { Fill } from '@wordpress/components';
import { createSlotFill } from '@wordpress/components';

export default function InspectorControls( { children } ) {
return (
<Fill name="Inspector.Controls">
{ children }
</Fill>
);
}
export default createSlotFill( 'InspectorControls' );
2 changes: 1 addition & 1 deletion components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export { default as ToggleControl } from './toggle-control';
export { default as Toolbar } from './toolbar';
export { default as Tooltip } from './tooltip';
export { default as TreeSelect } from './tree-select';
export { Slot, Fill, Provider as SlotFillProvider } from './slot-fill';
export { createSlotFill, Slot, Fill, Provider as SlotFillProvider } from './slot-fill';

// Higher-Order Components
export { default as ifCondition } from './higher-order/if-condition';
Expand Down
37 changes: 36 additions & 1 deletion components/slot-fill/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,48 @@ Any Fill will automatically occupy this Slot space, even if rendered elsewhere i

You can either use the Fill component directly, or a wrapper component type as in the above example to abstract the slot name from consumer awareness.

There is also `createSlotFill` helper method which was created to simplify the process of matching the corresponding `Slot` and `Fill` components:

```jsx
const Toolbar = createSlotFill( 'Toolbar' );

const MyToolbarItem = () => (
<Toolbar>
My item
</Toolbar>
);

const MyToolbar = () => (
<div className="toolbar">
<Toolbar.Slot />
</div>
);
```

## Props

The `SlotFillProvider` component does not accept any props.

Both `Slot` and `Fill` accept a `name` string prop, where a `Slot` with a given `name` will render the `children` of any associated `Fill`s.

`Slot` also accepts a `bubblesVirtually` prop which changes the event bubbling behaviour:
`Slot` accepts a `bubblesVirtually` prop which changes the event bubbling behaviour:

- By default, events will bubble to their parents on the DOM hierarchy (native event bubbling)
- If `bubblesVirtually` is set to true, events will bubble to their virtual parent in the React elements hierarchy instead.

`Slot` also accepts optional `children` function prop, which takes `fills` as a param. It allows to perform additional processing and wrap `fills` conditionally.

_Example_:
```jsx
const Toolbar = ( { isMobile } ) => (
<div className="toolbar">
<Slot name="Toolbar">
{ ( fills ) => {
return isMobile && fills.length > 3 ?
<div className="toolbar__mobile-long">{ fills }</div> :
fills;
} }
</Slot>
</div>
);
```
17 changes: 16 additions & 1 deletion components/slot-fill/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,19 @@ export { Slot };
export { Fill };
export { Provider };

export default { Slot, Fill, Provider };
export function createSlotFill( name ) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice helper method 👍 It simplifies a lot the slot fill creation.

const Component = ( { children, ...props } ) => (
<Fill name={ name } { ...props }>
{ children }
</Fill>
);
Component.displayName = name;

Component.Slot = ( { children, ...props } ) => (
<Slot name={ name } { ...props }>
{ children }
</Slot>
);

return Component;
}
38 changes: 20 additions & 18 deletions components/slot-fill/slot.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,32 +45,34 @@ class Slot extends Component {
}

render() {
const { name, bubblesVirtually = false, fillProps = {} } = this.props;
const { children, name, bubblesVirtually = false, fillProps = {} } = this.props;
const { getFills = noop } = this.context;

if ( bubblesVirtually ) {
return <div ref={ this.bindNode } />;
}

const fills = map( getFills( name ), ( fill ) => {
const fillKey = fill.occurrence;

// If a function is passed as a child, render it with the fillProps.
if ( isFunction( fill.props.children ) ) {
return cloneElement( fill.props.children( fillProps ), { key: fillKey } );
}

return Children.map( fill.props.children, ( child, childIndex ) => {
if ( ! child || isString( child ) ) {
return child;
}

const childKey = `${ fillKey }---${ child.key || childIndex }`;
return cloneElement( child, { key: childKey } );
} );
} );

return (
<div ref={ this.bindNode }>
{ map( getFills( name ), ( fill ) => {
const fillKey = fill.occurrence;

// If a function is passed as a child, render it with the fillProps.
if ( isFunction( fill.props.children ) ) {
return cloneElement( fill.props.children( fillProps ), { key: fillKey } );
}

return Children.map( fill.props.children, ( child, childIndex ) => {
if ( ! child || isString( child ) ) {
return child;
}

const childKey = `${ fillKey }---${ child.key || childIndex }`;
return cloneElement( child, { key: childKey } );
} );
} ) }
{ isFunction( children ) ? children( fills.filter( Boolean ) ) : fills }
Copy link
Member

Choose a reason for hiding this comment

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

Why do we filter 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.

To get rid of empty elements. Some fills render nothing based on Redux state. If you want to prevent rendering a slot wrapper it it has no fill which render something this is the way to go. In retrospective, I think this could be also done outside. Should we move it out or improve here and add some comments?

Copy link
Member

Choose a reason for hiding this comment

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

Comments would be good.

It was also strange to me that we filter only if the children is passed as a function, but not otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Also, filter( Boolean ) doesn't sound like a sufficient way to imitate React's treatment of "empty". For example, a child of 0 would be omitted here, but is a valid / non-empty child to render.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I will open PR with all your comments addressed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done #9371.

</div>
);
}
Expand Down
28 changes: 28 additions & 0 deletions components/slot-fill/test/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* External dependecies
*/
import { shallow } from 'enzyme';

/**
* Internal dependencies
*/
import { createSlotFill, Fill, Slot } from '../';

describe( 'createSlotFill', () => {
const SLOT_NAME = 'MyFill';
const MyFill = createSlotFill( SLOT_NAME );

test( 'should match snapshot for Fill', () => {
const wrapper = shallow( <MyFill /> );

expect( wrapper.type() ).toBe( Fill );
expect( wrapper ).toHaveProp( 'name', SLOT_NAME );
} );

test( 'should match snapshot for Slot', () => {
const wrapper = shallow( <MyFill.Slot /> );

expect( wrapper.type() ).toBe( Slot );
expect( wrapper ).toHaveProp( 'name', SLOT_NAME );
} );
} );
37 changes: 37 additions & 0 deletions components/slot-fill/test/slot.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { mount } from 'enzyme';
import { isEmpty } from 'lodash';

/**
* Internal dependencies
Expand Down Expand Up @@ -103,6 +104,42 @@ describe( 'Slot', () => {
expect( onClose ).toHaveBeenCalledTimes( 1 );
} );

it( 'should render empty Fills without HTML wrapper when render props used', () => {
const element = mount(
<Provider>
<Slot name="chicken">
{ ( fills ) => ( ! isEmpty( fills ) && (
<blockquote>
{ fills }
</blockquote>
) ) }
</Slot>
<Fill name="chicken" />
</Provider>
);

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

it( 'should render a string Fill with HTML wrapper when render props used', () => {
const element = mount(
<Provider>
<Slot name="chicken">
{ ( fills ) => ( fills && (
<blockquote>
{ fills }
</blockquote>
) ) }
</Slot>
<Fill name="chicken">
content
</Fill>
</Provider>
);

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

it( 'should re-render Slot when not bubbling virtually', () => {
const element = mount(
<Provider>
Expand Down
5 changes: 5 additions & 0 deletions edit-post/components/sidebar/block-inspector-panel/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,9 @@
.components-panel__body-toggle {
color: $dark-gray-500;
}

&.editor-block-inspector__advanced {
border-top: 1px solid $light-gray-500;
margin-bottom: -16px;
}
}
Loading