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

Components: Refactor SlotFill #19242

Merged
merged 31 commits into from
Feb 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6061ecd
SlotFill initial implementation
diegohaz Dec 19, 2019
7e38b5a
Add manifest-devhub.json
diegohaz Dec 19, 2019
d032bee
Accept as prop on Slot
diegohaz Dec 19, 2019
13b60ef
Update stories
diegohaz Dec 19, 2019
0c77a8a
Update README.md
diegohaz Dec 20, 2019
bd84603
Update code
diegohaz Dec 20, 2019
23d3abc
Add slot-fill2 entries to components index file
diegohaz Dec 20, 2019
229d432
Add unit tests
diegohaz Dec 20, 2019
7aff149
Merge branch 'master' into update/slot-fill
diegohaz Jan 29, 2020
e8bbb9b
Fix git conflicts
diegohaz Jan 29, 2020
df31e9c
Merge branch 'master' into update/slot-fill
diegohaz Feb 3, 2020
1fc8026
Lint code
diegohaz Feb 3, 2020
81c4741
Merge branch 'master' into update/slot-fill
diegohaz Feb 18, 2020
c6f6341
Update manifest.json
diegohaz Feb 18, 2020
7085a53
Try: replace <Slot bubblesVirtually /> by Slot2
diegohaz Feb 18, 2020
4ab2b9e
Set a default value for SlotFillContext
diegohaz Feb 18, 2020
ba2ea9f
Update SlotFillContext slots initial value
diegohaz Feb 18, 2020
2db94e9
Refactor code
diegohaz Feb 19, 2020
a275949
Update docs/manifest.json
diegohaz Feb 19, 2020
0b11256
Merge branch 'master' into update/slot-fill
diegohaz Feb 19, 2020
94020b0
Update story title separator
diegohaz Feb 19, 2020
fcf9398
Update snapshots
diegohaz Feb 19, 2020
104335e
Remove bubblesVirtually implementation from BaseFill/Slot
diegohaz Feb 19, 2020
aa61605
Make sure fills are being created in the right order
diegohaz Feb 21, 2020
d1bfe35
Merge branch 'master' into update/slot-fill
diegohaz Feb 21, 2020
64987ca
Add comment on Fill dual rendering
diegohaz Feb 21, 2020
7c3e5fc
Add code comments on Fill
diegohaz Feb 22, 2020
2b31dbd
Try with compareDocumentPosition
diegohaz Feb 23, 2020
04f44ce
Revert ordering feature
diegohaz Feb 23, 2020
ce97491
Uncomment test
diegohaz Feb 25, 2020
d3ebcc2
Fix top toolbar not updating properly
diegohaz Feb 25, 2020
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
29 changes: 13 additions & 16 deletions packages/block-editor/src/components/block-inspector/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
} from '@wordpress/blocks';
import {
PanelBody,
__experimentalSlotFillConsumer,
__experimentalUseSlot as useSlot,
} from '@wordpress/components';
import { withSelect } from '@wordpress/data';

Expand All @@ -30,6 +30,9 @@ const BlockInspector = ( {
selectedBlockName,
showNoBlockSelectedMessage = true,
} ) => {
const slot = useSlot( InspectorAdvancedControls.slotName );
Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's keep __experimentalUseSlot experimental until we come up with a better way to handle it. It seems fine for the time being.

const hasFills = Boolean( slot.fills && slot.fills.length );

if ( count > 1 ) {
return <MultiSelectionInspector />;
}
Expand Down Expand Up @@ -69,21 +72,15 @@ const BlockInspector = ( {
) }
<InspectorControls.Slot bubblesVirtually />
<div>
<__experimentalSlotFillConsumer>
{ ( { hasFills } ) =>
hasFills( InspectorAdvancedControls.slotName ) && (
<PanelBody
className="block-editor-block-inspector__advanced"
title={ __( 'Advanced' ) }
initialOpen={ false }
>
<InspectorAdvancedControls.Slot
bubblesVirtually
/>
</PanelBody>
)
}
</__experimentalSlotFillConsumer>
{ hasFills && (
<PanelBody
className="block-editor-block-inspector__advanced"
title={ __( 'Advanced' ) }
initialOpen={ false }
>
<InspectorAdvancedControls.Slot bubblesVirtually />
</PanelBody>
) }
Comment on lines +75 to +83
Copy link
Member Author

Choose a reason for hiding this comment

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

Related discussion: #16807 (review)

</div>
<SkipToSelectedBlock key="back" />
</div>
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export {
Slot,
Fill,
Provider as SlotFillProvider,
Consumer as __experimentalSlotFillConsumer,
useSlot as __experimentalUseSlot,
diegohaz marked this conversation as resolved.
Show resolved Hide resolved
} from './slot-fill';

// Higher-Order Components
Expand Down
27 changes: 9 additions & 18 deletions packages/components/src/popover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import PopoverDetectOutside from './detect-outside';
import Button from '../button';
import ScrollLock from '../scroll-lock';
import IsolatedEventContainer from '../isolated-event-container';
import { Slot, Fill, Consumer } from '../slot-fill';
import { Slot, Fill, useSlot } from '../slot-fill';
import Animate from '../animate';

const FocusManaged = withConstrainedTabbing(
Expand Down Expand Up @@ -261,6 +261,7 @@ const Popover = ( {
const contentRect = useRef();
const isMobileViewport = useViewportMatch( 'medium', '<' );
const [ animateOrigin, setAnimateOrigin ] = useState();
const slot = useSlot( __unstableSlotName );
const isExpanded = expandOnMobile && isMobileViewport;

noArrow = isExpanded || noArrow;
Expand Down Expand Up @@ -602,25 +603,15 @@ const Popover = ( {
content = <FocusManaged>{ content }</FocusManaged>;
}

return (
<Consumer>
{ ( { getSlot } ) => {
// In case there is no slot context in which to render,
// default to an in-place rendering.
if ( getSlot && getSlot( __unstableSlotName ) ) {
content = (
<Fill name={ __unstableSlotName }>{ content }</Fill>
);
}
if ( slot.ref ) {
content = <Fill name={ __unstableSlotName }>{ content }</Fill>;
}

if ( anchorRef || anchorRect ) {
return content;
}
if ( anchorRef || anchorRect ) {
return content;
}

return <span ref={ anchorRefFallback }>{ content }</span>;
} }
</Consumer>
);
return <span ref={ anchorRefFallback }>{ content }</span>;
Comment on lines +606 to +614
Copy link
Member Author

@diegohaz diegohaz Feb 19, 2020

Choose a reason for hiding this comment

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

Looks like PopoverSlot always uses bubblesVirtually, so I also changed this snippet to use useSlot instead of Consumer. With that, Consumer is only used on the SlotFill module internally.

};

const PopoverContainer = Popover;
Expand Down
34 changes: 34 additions & 0 deletions packages/components/src/slot-fill/bubbles-virtually/fill.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* WordPress dependencies
*/
import { useRef, useEffect, createPortal } from '@wordpress/element';

/**
* Internal dependencies
*/
import useSlot from './use-slot';

export default function Fill( { name, children } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a simple and nice implementation but I believe we had issues with fills being reordered when we switch between blocks (toolbar and inspector) and we had to add a custom logic to keep the fills rendering in the exact same order they appear on the React tree (even with conditions).

I see that this new implementation passes the existing unit tests right which I believe account for this (and I'm surprised it does without any tweaks to the rendering order). We might just want to confirm in the UI too.

Copy link
Member Author

@diegohaz diegohaz Feb 19, 2020

Choose a reason for hiding this comment

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

Hmm, from what I can see, the current unit tests are not covering that case for bubblesVirtually. Actually, it seems that bubblesVirtually is not being covered at all? react-test-renderer doesn't seem to support portals. But I'll check if react-dom/test-utils can do the job.

Also, I can't find how it's been solved for the current implementation (using bubblesVirtually). Do you have an idea?

I'm pretty sure that my implementation doesn't cover that. ReactDOM.createPortal, by default, will just attach elements in the order they're rendered.

I've just made some experiments with the key prop here, which seems promising: https://codesandbox.io/s/keeping-react-portal-order-uk9xb

Copy link
Contributor

Choose a reason for hiding this comment

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

It's related to the occurrence counter we have on fills and there's a function to reorder the fills in the Provider I believe. That said, If we manage to run the unit tests on the new implementation, that would be enough to validate it I think.

const slot = useSlot( name );
const ref = useRef();

useEffect( () => {
// We register fills so we can keep track of their existance.
// Some Slot implementations need to know if there're already fills
// registered so they can choose to render themselves or not.
slot.registerFill( ref );
return () => {
slot.unregisterFill( ref );
};
}, [ slot.registerFill, slot.unregisterFill ] );

if ( ! slot.ref || ! slot.ref.current ) {
return null;
}

if ( typeof children === 'function' ) {
children = children( slot.fillProps );
}

return createPortal( children, slot.ref.current );
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* WordPress dependencies
*/
import { createContext } from '@wordpress/element';

const SlotFillContext = createContext( {
slots: {},
fills: {},
registerSlot: () => {},
unregisterSlot: () => {},
registerFill: () => {},
unregisterFill: () => {},
} );

export default SlotFillContext;
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/**
* WordPress dependencies
*/
import { useMemo, useCallback, useState } from '@wordpress/element';

/**
* Internal dependencies
*/
import SlotFillContext from './slot-fill-context';

function useSlotRegistry() {
const [ slots, setSlots ] = useState( {} );
const [ fills, setFills ] = useState( {} );

const registerSlot = useCallback( ( name, ref, fillProps ) => {
setSlots( ( prevSlots ) => ( {
...prevSlots,
[ name ]: {
...prevSlots[ name ],
ref: ref || prevSlots[ name ].ref,
fillProps: fillProps || prevSlots[ name ].fillProps || {},
},
} ) );
}, [] );

const unregisterSlot = useCallback( ( name, ref ) => {
setSlots( ( prevSlots ) => {
// eslint-disable-next-line no-unused-vars
const { [ name ]: slot, ...nextSlots } = prevSlots;
// Make sure we're not unregistering a slot registered by another element
// See https://github.com/WordPress/gutenberg/pull/19242#issuecomment-590295412
if ( slot.ref === ref ) {
Comment on lines +29 to +32
Copy link
Member

Choose a reason for hiding this comment

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

Should it be safe to assume slot is defined here?

In debugging the issue mentioned by @draganescu at #18560 (comment) , the exact error occurs at this line of code:

image

It's not yet clear to me if the bug is in how the slot came to be undefined in the first place, or if we should be guarded in how we expect slot to be available at this line of code.

return nextSlots;
}
return prevSlots;
} );
}, [] );

const registerFill = useCallback( ( name, ref ) => {
setFills( ( prevFills ) => ( {
...prevFills,
[ name ]: [ ...( prevFills[ name ] || [] ), ref ],
} ) );
}, [] );

const unregisterFill = useCallback( ( name, ref ) => {
setFills( ( prevFills ) => {
if ( prevFills[ name ] ) {
return {
...prevFills,
[ name ]: prevFills[ name ].filter(
( fillRef ) => fillRef !== ref
),
};
}
return prevFills;
} );
}, [] );

// Memoizing the return value so it can be directly passed to Provider value
const registry = useMemo(
() => ( {
slots,
fills,
registerSlot,
// Just for readability
updateSlot: registerSlot,
unregisterSlot,
registerFill,
unregisterFill,
} ),
[
slots,
fills,
registerSlot,
unregisterSlot,
registerFill,
unregisterFill,
]
);

return registry;
}

export default function SlotFillProvider( { children } ) {
const registry = useSlotRegistry();
return (
<SlotFillContext.Provider value={ registry }>
{ children }
</SlotFillContext.Provider>
);
}
48 changes: 48 additions & 0 deletions packages/components/src/slot-fill/bubbles-virtually/slot.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/**
* WordPress dependencies
*/
import {
useEffect,
useRef,
useLayoutEffect,
useContext,
} from '@wordpress/element';
import isShallowEqual from '@wordpress/is-shallow-equal';

/**
* Internal dependencies
*/
import SlotFillContext from './slot-fill-context';
import useSlot from './use-slot';

export default function Slot( {
name,
fillProps = {},
as: Component = 'div',
...props
} ) {
const registry = useContext( SlotFillContext );
const ref = useRef();
const slot = useSlot( name );

useEffect( () => {
registry.registerSlot( name, ref, fillProps );
return () => {
registry.unregisterSlot( name, ref );
};
// We are not including fillProps in the deps because we don't want to
// unregister and register the slot whenever fillProps change, which would
// cause the fill to be re-mounted. We are only considering the initial value
// of fillProps.
}, [ registry.registerSlot, registry.unregisterSlot, name ] );

// fillProps may be an update that interact with the layout, so
// we useLayoutEffect
useLayoutEffect( () => {
if ( slot.fillProps && ! isShallowEqual( slot.fillProps, fillProps ) ) {
registry.updateSlot( name, ref, fillProps );
}
} );

return <Component ref={ ref } { ...props } />;
}
54 changes: 54 additions & 0 deletions packages/components/src/slot-fill/bubbles-virtually/use-slot.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/**
* WordPress dependencies
*/
import { useCallback, useContext, useMemo } from '@wordpress/element';

/**
* Internal dependencies
*/
import SlotFillContext from './slot-fill-context';

export default function useSlot( name ) {
const registry = useContext( SlotFillContext );

const slot = registry.slots[ name ] || {};
const slotFills = registry.fills[ name ];
const fills = useMemo( () => slotFills || [], [ slotFills ] );

const updateSlot = useCallback(
( slotRef, slotFillProps ) => {
registry.updateSlot( name, slotRef, slotFillProps );
},
[ name, registry.updateSlot ]
);

const unregisterSlot = useCallback(
( slotRef ) => {
registry.unregisterSlot( name, slotRef );
},
[ name, registry.unregisterSlot ]
);

const registerFill = useCallback(
( fillRef ) => {
registry.registerFill( name, fillRef );
},
[ name, registry.registerFill ]
);

const unregisterFill = useCallback(
( fillRef ) => {
registry.unregisterFill( name, fillRef );
},
[ name, registry.unregisterFill ]
);

return {
...slot,
updateSlot,
unregisterSlot,
fills,
registerFill,
unregisterFill,
};
}
9 changes: 8 additions & 1 deletion packages/components/src/slot-fill/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ import {
useEffect,
} from '@wordpress/element';

/**
* Internal dependencies
*/
import SlotFillBubblesVirtuallyProvider from './bubbles-virtually/slot-fill-provider';

const SlotFillContext = createContext( {
registerSlot: () => {},
unregisterSlot: () => {},
Expand Down Expand Up @@ -140,7 +145,9 @@ class SlotFillProvider extends Component {
render() {
return (
<Provider value={ this.contextValue }>
{ this.props.children }
<SlotFillBubblesVirtuallyProvider>
{ this.props.children }
</SlotFillBubblesVirtuallyProvider>
</Provider>
);
}
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/slot-fill/fill.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ function FillComponent( { name, children, registerFill, unregisterFill } ) {

useLayoutEffect( () => {
ref.current.children = children;
if ( slot && ! slot.props.bubblesVirtually ) {
if ( slot ) {
slot.forceUpdate();
}
}, [ children ] );
Expand All @@ -49,7 +49,7 @@ function FillComponent( { name, children, registerFill, unregisterFill } ) {
registerFill( name, ref.current );
}, [ name ] );

if ( ! slot || ! slot.node || ! slot.props.bubblesVirtually ) {
if ( ! slot || ! slot.node ) {
return null;
}

Expand Down
Loading