From bdba6b8771511b4fe30b8e4801fe77343bd3cc72 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 14 Dec 2018 12:13:39 -0500 Subject: [PATCH] Components: Handle multiple Slots by same name --- packages/components/CHANGELOG.md | 6 +++ packages/components/src/slot-fill/context.js | 26 ++++++++- packages/components/src/slot-fill/fill.js | 2 +- packages/components/src/slot-fill/slot.js | 2 +- .../slot-fill/test/__snapshots__/slot.js.snap | 54 +++++++++++++++++++ .../components/src/slot-fill/test/slot.js | 44 +++++++++++++++ 6 files changed, 130 insertions(+), 4 deletions(-) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 8dfd644e15ea0c..87b8d56b8c9754 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -1,3 +1,9 @@ +## 7.0.5 (Unreleased) + +### Bug Fixes + +- Resolves a conflict where two instance of Slot would produce an inconsistent or duplicated rendering output. + ## 7.0.4 (2018-12-12) ## 7.0.3 (2018-11-30) diff --git a/packages/components/src/slot-fill/context.js b/packages/components/src/slot-fill/context.js index df4476fea5b9ce..a39ac94013e0af 100644 --- a/packages/components/src/slot-fill/context.js +++ b/packages/components/src/slot-fill/context.js @@ -41,12 +41,21 @@ class SlotFillProvider extends Component { } registerSlot( name, slot ) { + const previousSlot = this.slots[ name ]; this.slots[ name ] = slot; this.forceUpdateFills( name ); // 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 ); + + // If a new instance of a slot is being mounted while another with the + // same name exists, force its update _after_ the new slot has been + // assigned into the instance, such that its own rendering of children + // will be empty (the new Slot will subsume all fills for this name). + if ( previousSlot ) { + previousSlot.forceUpdate(); + } } registerFill( name, instance ) { @@ -57,7 +66,14 @@ class SlotFillProvider extends Component { this.forceUpdateSlot( name ); } - unregisterSlot( name ) { + unregisterSlot( name, instance ) { + // If a previous instance of a Slot by this name unmounts, do nothing, + // as the slot and its fills should only be removed for the current + // known instance. + if ( this.slots[ name ] !== instance ) { + return; + } + delete this.slots[ name ]; this.forceUpdateFills( name ); } @@ -75,7 +91,13 @@ class SlotFillProvider extends Component { return this.slots[ name ]; } - getFills( name ) { + getFills( name, slotInstance ) { + // Fills should only be returned for the current instance of the slot + // in which they occupy. + if ( this.slots[ name ] !== slotInstance ) { + return []; + } + return sortBy( this.fills[ name ], 'occurrence' ); } diff --git a/packages/components/src/slot-fill/fill.js b/packages/components/src/slot-fill/fill.js index 66b025ca001788..7c2b3fb0ad5a28 100644 --- a/packages/components/src/slot-fill/fill.js +++ b/packages/components/src/slot-fill/fill.js @@ -62,7 +62,7 @@ class FillComponent extends Component { let { children } = this.props; const slot = getSlot( name ); - if ( ! slot || ! slot.props.bubblesVirtually ) { + if ( ! slot || ! slot.node || ! slot.props.bubblesVirtually ) { return null; } diff --git a/packages/components/src/slot-fill/slot.js b/packages/components/src/slot-fill/slot.js index 4f53cd6d103004..13a226eab5de5d 100644 --- a/packages/components/src/slot-fill/slot.js +++ b/packages/components/src/slot-fill/slot.js @@ -63,7 +63,7 @@ class SlotComponent extends Component { return
; } - const fills = map( getFills( name ), ( fill ) => { + const fills = map( getFills( name, this ), ( fill ) => { const fillKey = fill.occurrence; const fillChildren = isFunction( fill.props.children ) ? fill.props.children( fillProps ) : fill.props.children; diff --git a/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap b/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap index 709d1522fcbf4c..79b43ad4c65c07 100644 --- a/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap +++ b/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap @@ -1,5 +1,59 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`Slot bubblesVirtually false should subsume another slot by the same name 1`] = ` +Array [ +
, +
+ Content +
, +] +`; + +exports[`Slot bubblesVirtually false should subsume another slot by the same name 2`] = ` +Array [ +
, +
+ Content +
, +] +`; + +exports[`Slot bubblesVirtually true should subsume another slot by the same name 1`] = ` +Array [ +
+
+
, +
+
+
, +] +`; + +exports[`Slot bubblesVirtually true should subsume another slot by the same name 2`] = ` +Array [ +
, +
+
+
, +] +`; + exports[`Slot should re-render Slot when not bubbling virtually 1`] = ` Array [
diff --git a/packages/components/src/slot-fill/test/slot.js b/packages/components/src/slot-fill/test/slot.js index a12c9cd99c6cab..47ba2e16de5247 100644 --- a/packages/components/src/slot-fill/test/slot.js +++ b/packages/components/src/slot-fill/test/slot.js @@ -209,4 +209,48 @@ describe( 'Slot', () => { expect( testRenderer.toJSON() ).toMatchSnapshot(); } ); + + [ false, true ].forEach( ( bubblesVirtually ) => { + describe( 'bubblesVirtually ' + bubblesVirtually, () => { + it( 'should subsume another slot by the same name', () => { + const testRenderer = ReactTestRenderer.create( + +
+ +
+
+ Content + + ); + + testRenderer.update( + +
+ +
+
+ +
+ Content + + ); + + expect( testRenderer.toJSON() ).toMatchSnapshot(); + + testRenderer.update( + +
+
+ +
+ Content + + ); + + expect( testRenderer.toJSON() ).toMatchSnapshot(); + + expect( testRenderer.getInstance().slots ).toHaveProperty( 'egg' ); + } ); + } ); + } ); } );