From 767dc2d9668c74b32dabf5a02e9c067d7b51841f Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 24 Aug 2018 11:24:36 -0400 Subject: [PATCH 1/2] Testing: Update Slot tests to use react-test-renderer --- .../components/src/slot-fill/test/slot.js | 129 +++++++++++------- 1 file changed, 80 insertions(+), 49 deletions(-) diff --git a/packages/components/src/slot-fill/test/slot.js b/packages/components/src/slot-fill/test/slot.js index 48b2bc805dd3a3..f2fc45aee142c2 100644 --- a/packages/components/src/slot-fill/test/slot.js +++ b/packages/components/src/slot-fill/test/slot.js @@ -1,9 +1,14 @@ /** * External dependencies */ -import { mount } from 'enzyme'; +import { create } from 'react-test-renderer'; import { isEmpty } from 'lodash'; +/** + * WordPress Dependencies + */ +import { Component } from '@wordpress/element'; + /** * Internal dependencies */ @@ -11,11 +16,6 @@ import Slot from '../slot'; import Fill from '../fill'; import Provider from '../provider'; -/** - * WordPress Dependencies - */ -import { Component } from '@wordpress/element'; - class Filler extends Component { constructor() { super( ...arguments ); @@ -33,19 +33,25 @@ class Filler extends Component { } describe( 'Slot', () => { + function expectEmptyFill( wrapper ) { + expect( wrapper.root.findByType( Fill ).children ).toHaveLength( 0 ); + } + it( 'should render empty Fills', () => { - const element = mount( + const wrapper = create( ); - expect( element.find( 'Slot > div' ).html() ).toBe( '
' ); + const slot = wrapper.root.findByType( Slot ); + expect( slot.children ).toHaveLength( 0 ); + expectEmptyFill( wrapper ); } ); it( 'should render a string Fill', () => { - const element = mount( + const wrapper = create( @@ -54,11 +60,14 @@ describe( 'Slot', () => { ); - expect( element.find( 'Slot > div' ).html() ).toBe( '
content
' ); + const slot = wrapper.root.findByType( Slot ); + expect( slot.children ).toHaveLength( 1 ); + expect( slot.children[ 0 ] ).toBe( 'content' ); + expectEmptyFill( wrapper ); } ); it( 'should render a Fill containing an element', () => { - const element = mount( + const wrapper = create( @@ -67,11 +76,14 @@ describe( 'Slot', () => { ); - expect( element.find( 'Slot > div' ).html() ).toBe( '
' ); + const slot = wrapper.root.findByType( Slot ); + expect( slot.children ).toHaveLength( 1 ); + expect( slot.children[ 0 ].type ).toBe( 'span' ); + expectEmptyFill( wrapper ); } ); it( 'should render a Fill containing an array', () => { - const element = mount( + const wrapper = create( @@ -80,13 +92,18 @@ describe( 'Slot', () => { ); - expect( element.find( 'Slot > div' ).html() ).toBe( '
text
' ); + const slot = wrapper.root.findByType( Slot ); + expect( slot.children ).toHaveLength( 3 ); + expect( slot.children[ 0 ].type ).toBe( 'span' ); + expect( slot.children[ 1 ].type ).toBe( 'div' ); + expect( slot.children[ 2 ] ).toBe( 'text' ); + expectEmptyFill( wrapper ); } ); it( 'calls the functions passed as the Slot’s fillProps in the Fill', () => { const onClose = jest.fn(); - const element = mount( + const wrapper = create( @@ -99,13 +116,13 @@ describe( 'Slot', () => { ); - element.find( 'button' ).simulate( 'click' ); + wrapper.root.findByType( Slot ).findByType( 'button' ).props.onClick(); expect( onClose ).toHaveBeenCalledTimes( 1 ); } ); it( 'should render empty Fills without HTML wrapper when render props used', () => { - const element = mount( + const wrapper = create( { ( fills ) => ( ! isEmpty( fills ) && ( @@ -118,11 +135,13 @@ describe( 'Slot', () => { ); - expect( element.find( 'Slot > div' ).html() ).toBe( '
' ); + const slot = wrapper.root.findByType( Slot ); + expect( slot.children ).toHaveLength( 0 ); + expectEmptyFill( wrapper ); } ); it( 'should render a string Fill with HTML wrapper when render props used', () => { - const element = mount( + const wrapper = create( { ( fills ) => ( fills && ( @@ -137,54 +156,66 @@ describe( 'Slot', () => { ); - expect( element.find( 'Slot > div' ).html() ).toBe( '
content
' ); + const slot = wrapper.root.findByType( Slot ); + expect( slot.children ).toHaveLength( 1 ); + expect( slot.children[ 0 ].type ).toBe( 'blockquote' ); + expect( slot.children[ 0 ].children ).toHaveLength( 1 ); + expect( slot.children[ 0 ].children[ 0 ] ).toBe( 'content' ); + expectEmptyFill( wrapper ); } ); it( 'should re-render Slot when not bubbling virtually', () => { - const element = mount( + const wrapper = create( ); - expect( element.find( 'Slot > div' ).html() ).toBe( '
1
' ); + let slot = wrapper.root.findByType( Slot ); + expect( slot.children ).toHaveLength( 1 ); + expect( slot.children[ 0 ] ).toBe( '1' ); - element.find( 'button' ).simulate( 'click' ); + wrapper.root.findByType( Filler ).findByType( 'button' ).props.onClick(); - expect( element.find( 'Slot > div' ).html() ).toBe( '
2
' ); + slot = wrapper.root.findByType( Slot ); + expect( slot.children ).toHaveLength( 1 ); + expect( slot.children[ 0 ] ).toBe( '2' ); } ); it( 'should render in expected order', () => { - const element = mount( + const wrapper = create( + + + + ); + + wrapper.update( + + + + + + ); + + wrapper.update( + + + + + ); + + wrapper.update( + + ); - element.setProps( { - children: [ - , - , - , - ], - } ); - - element.setProps( { - children: [ - , - , - ], - } ); - - element.setProps( { - children: [ - , - , - , - ], - } ); - - expect( element.find( 'Slot > div' ).html() ).toBe( '
firstsecond
' ); + const slot = wrapper.root.findByType( Slot ); + expect( slot.children ).toHaveLength( 2 ); + expect( slot.children[ 0 ] ).toBe( 'first' ); + expect( slot.children[ 1 ] ).toBe( 'second' ); } ); } ); From 09a31f73fcff039314c19956dc45c627a3a2ada0 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 24 Aug 2018 15:46:25 -0400 Subject: [PATCH 2/2] Components: Avoid wrapper element for non-virtual Slot --- .../test/__snapshots__/index.js.snap | 86 +++++++++--------- .../test/__snapshots__/index.js.snap | 20 ++--- .../test/__snapshots__/index.js.snap | 86 +++++++++--------- packages/components/src/slot-fill/fill.js | 32 ++++++- packages/components/src/slot-fill/provider.js | 27 ++++-- packages/components/src/slot-fill/slot.js | 25 +++--- .../components/src/slot-fill/test/slot.js | 87 +++++++++++++++++++ 7 files changed, 243 insertions(+), 120 deletions(-) diff --git a/edit-post/components/sidebar/plugin-post-publish-panel/test/__snapshots__/index.js.snap b/edit-post/components/sidebar/plugin-post-publish-panel/test/__snapshots__/index.js.snap index f35ccd569e3fb3..81f7de035367d3 100644 --- a/edit-post/components/sidebar/plugin-post-publish-panel/test/__snapshots__/index.js.snap +++ b/edit-post/components/sidebar/plugin-post-publish-panel/test/__snapshots__/index.js.snap @@ -1,57 +1,53 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP exports[`PluginPostPublishPanel renders fill properly 1`] = ` -
- -
-

- - -

- My panel content -
- -
+ + + + + + + + My panel title + + + + My panel content + + `; diff --git a/edit-post/components/sidebar/plugin-post-status-info/test/__snapshots__/index.js.snap b/edit-post/components/sidebar/plugin-post-status-info/test/__snapshots__/index.js.snap index 9acda7281b2351..4b79a2c7a08529 100644 --- a/edit-post/components/sidebar/plugin-post-status-info/test/__snapshots__/index.js.snap +++ b/edit-post/components/sidebar/plugin-post-status-info/test/__snapshots__/index.js.snap @@ -1,18 +1,14 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP exports[`PluginPostStatusInfo renders fill properly 1`] = ` -
- -
- My plugin post status info -
- -
+ My plugin post status info + + `; diff --git a/edit-post/components/sidebar/plugin-pre-publish-panel/test/__snapshots__/index.js.snap b/edit-post/components/sidebar/plugin-pre-publish-panel/test/__snapshots__/index.js.snap index cb24a9396aff69..11b2c1b884d54d 100644 --- a/edit-post/components/sidebar/plugin-pre-publish-panel/test/__snapshots__/index.js.snap +++ b/edit-post/components/sidebar/plugin-pre-publish-panel/test/__snapshots__/index.js.snap @@ -1,57 +1,53 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP exports[`PluginPrePublishPanel renders fill properly 1`] = ` -
- -
-

- - -

- My panel content -
- -
+ + + + + + + + My panel title + + + + My panel content + + `; diff --git a/packages/components/src/slot-fill/fill.js b/packages/components/src/slot-fill/fill.js index 27f8da4c6fa852..9cc3c9f551226b 100644 --- a/packages/components/src/slot-fill/fill.js +++ b/packages/components/src/slot-fill/fill.js @@ -20,6 +20,8 @@ class Fill extends Component { const { registerFill = noop } = this.context; registerFill( this.props.name, this ); + + this.checkIfSlotAvailable(); } componentWillUpdate() { @@ -50,6 +52,23 @@ class Fill extends Component { unregisterFill( prevProps.name, this ); registerFill( name, this ); } + + this.checkIfSlotAvailable(); + } + + /** + * Forces an update if the target slot becomes available after the fill was + * rendered without the slot having yet been prepared. This can occur when + * the slot is mounted after the fill, or when the slot and fill render + * simultaneously and the Slot's portal target has not yet been mounted for + * the fill to use as a container. + */ + checkIfSlotAvailable() { + const { getSlot = noop } = this.context; + const { name } = this.props; + if ( this.isPendingSlot && !! getSlot( name ) ) { + this.forceUpdate(); + } } resetOccurrence() { @@ -60,9 +79,18 @@ class Fill extends Component { const { getSlot = noop } = this.context; const { name } = this.props; let { children } = this.props; + const slot = getSlot( name ); + if ( ! slot ) { + this.isPendingSlot = true; + return null; + } + + delete this.isPendingSlot; - if ( ! slot || ! slot.props.bubblesVirtually ) { + // A slot which does not bubble events virtually is responsible for + // rendering its own fills. + if ( ! slot.props.bubblesVirtually ) { return null; } @@ -71,7 +99,7 @@ class Fill extends Component { children = children( slot.props.fillProps ); } - return createPortal( children, slot.node ); + return createPortal( children, slot.virtualTarget.current ); } } diff --git a/packages/components/src/slot-fill/provider.js b/packages/components/src/slot-fill/provider.js index 29f5555244cb67..7a07d3322fbf42 100644 --- a/packages/components/src/slot-fill/provider.js +++ b/packages/components/src/slot-fill/provider.js @@ -36,11 +36,19 @@ class SlotFillProvider extends Component { registerSlot( name, slot ) { this.slots[ name ] = slot; - this.forceUpdateFills( name ); + + // TODO: Track down when this case occurs, see if we can at least limit + // it to specific variations of Slot/Fill. Needs test case. // Sometimes the fills are registered after the initial render of slot // But before the registerSlot call, we need to rerender the slot - this.forceUpdateSlot( name ); + this.forceUpdateFillRenderingSlot( name ); + + // A fill in a bubblesVirtually slot renders itself via createPortal. + // If the slot was mounted after the fill, the fill needs update. + if ( slot.props.bubblesVirtually ) { + this.forceUpdateFills( name ); + } } registerFill( name, instance ) { @@ -48,7 +56,7 @@ class SlotFillProvider extends Component { ...( this.fills[ name ] || [] ), instance, ]; - this.forceUpdateSlot( name ); + this.forceUpdateFillRenderingSlot( name ); } unregisterSlot( name ) { @@ -62,7 +70,7 @@ class SlotFillProvider extends Component { instance ); this.resetFillOccurrence( name ); - this.forceUpdateSlot( name ); + this.forceUpdateFillRenderingSlot( name ); } getSlot( name ) { @@ -85,10 +93,17 @@ class SlotFillProvider extends Component { } ); } - forceUpdateSlot( name ) { + /** + * Forces update of a slot, if the slot is responsible for rendering fills + * of the given name. A slot renders its fills if its bubblesVirtually prop + * is not `true`; otherwise, the fill renders itself by createPortal. + * + * @param {string} name Name of fill-rendering slot to update. + */ + forceUpdateFillRenderingSlot( name ) { const slot = this.getSlot( name ); - if ( slot ) { + if ( slot && ! slot.props.bubblesVirtually ) { slot.forceUpdate(); } } diff --git a/packages/components/src/slot-fill/slot.js b/packages/components/src/slot-fill/slot.js index cf6a51cb1ca188..133eb92a9f95a0 100644 --- a/packages/components/src/slot-fill/slot.js +++ b/packages/components/src/slot-fill/slot.js @@ -6,13 +6,19 @@ import { noop, map, isString, isFunction } from 'lodash'; /** * WordPress dependencies */ -import { Component, Children, cloneElement } from '@wordpress/element'; +import { + Component, + Children, + Fragment, + cloneElement, + createRef, +} from '@wordpress/element'; class Slot extends Component { constructor() { super( ...arguments ); - this.bindNode = this.bindNode.bind( this ); + this.virtualTarget = createRef(); } componentDidMount() { @@ -40,16 +46,15 @@ class Slot extends Component { } } - bindNode( node ) { - this.node = node; - } - render() { - const { children, name, bubblesVirtually = false, fillProps = {} } = this.props; + const { children, name, bubblesVirtually, fillProps = {} } = this.props; const { getFills = noop } = this.context; if ( bubblesVirtually ) { - return
; + // When bubbling virtually, createPortal requires a DOM node in + // which to render the fill elements. This `div` serves no purpose + // other than to act as the target container. + return
; } const fills = map( getFills( name ), ( fill ) => { @@ -67,9 +72,9 @@ class Slot extends Component { } ); return ( -
+ { isFunction( children ) ? children( fills.filter( Boolean ) ) : fills } -
+ ); } } diff --git a/packages/components/src/slot-fill/test/slot.js b/packages/components/src/slot-fill/test/slot.js index f2fc45aee142c2..a20972ee7619af 100644 --- a/packages/components/src/slot-fill/test/slot.js +++ b/packages/components/src/slot-fill/test/slot.js @@ -2,6 +2,11 @@ * External dependencies */ import { create } from 'react-test-renderer'; +import { + renderIntoDocument, + findRenderedComponentWithType, + findRenderedDOMComponentWithTag, +} from 'react-dom/test-utils'; import { isEmpty } from 'lodash'; /** @@ -100,6 +105,53 @@ describe( 'Slot', () => { expectEmptyFill( wrapper ); } ); + it( 'should render a Fill to a Slot mounted later', () => { + const wrapper = create( + + + content + + + ); + + wrapper.update( + + + content + + + + ); + + const slot = wrapper.root.findByType( Slot ); + expect( slot.children ).toHaveLength( 1 ); + expect( slot.children[ 0 ] ).toBe( 'content' ); + expectEmptyFill( wrapper ); + } ); + + it( 'should render a Fill to a Slot mounted later (virtually)', () => { + let tree = renderIntoDocument( + + + content + + + ); + + tree = renderIntoDocument( + + + content + + + + ); + + const slot = findRenderedComponentWithType( tree, Slot ); + const slotDiv = findRenderedDOMComponentWithTag( slot, 'div' ); + expect( slotDiv.outerHTML ).toBe( '
content
' ); + } ); + it( 'calls the functions passed as the Slot’s fillProps in the Fill', () => { const onClose = jest.fn(); @@ -218,4 +270,39 @@ describe( 'Slot', () => { expect( slot.children[ 0 ] ).toBe( 'first' ); expect( slot.children[ 1 ] ).toBe( 'second' ); } ); + + it( 'should unrender unmounted fills', () => { + const wrapper = create( + + + first + + ); + + wrapper.update( + + + second + + ); + + const slot = wrapper.root.findByType( Slot ); + expect( slot.children ).toHaveLength( 1 ); + expect( slot.children[ 0 ] ).toBe( 'second' ); + } ); + + it( 'should render virtually by portal', () => { + const tree = renderIntoDocument( + + + + content + + + ); + + const slot = findRenderedComponentWithType( tree, Slot ); + const slotDiv = findRenderedDOMComponentWithTag( slot, 'div' ); + expect( slotDiv.outerHTML ).toBe( '
content
' ); + } ); } );