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

SlotFill: unify registry and fill implementation #68056

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import {
fireEvent,
render,
render as baseRender,
screen,
waitFor,
within,
Expand All @@ -13,6 +13,7 @@ import userEvent from '@testing-library/user-event';
/**
* WordPress dependencies
*/
import { SlotFillProvider } from '@wordpress/components';
import { useState } from '@wordpress/element';
import { useSelect } from '@wordpress/data';

Expand Down Expand Up @@ -67,6 +68,10 @@ afterEach( () => {
mockFetchRichUrlData?.mockReset(); // Conditionally reset as it may NOT be a mock.
} );

function render( ui, options ) {
return baseRender( ui, { wrapper: SlotFillProvider, ...options } );
Comment on lines +71 to +72
Copy link
Member

Choose a reason for hiding this comment

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

Can't we remove options altogether?

Suggested change
function render( ui, options ) {
return baseRender( ui, { wrapper: SlotFillProvider, ...options } );
function render( ui ) {
return baseRender( ui, { wrapper: SlotFillProvider } );

}

/**
* Workaround to trigger an arrow up keypress event.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import userEvent from '@testing-library/user-event';
* WordPress dependencies
*/
import { useState } from '@wordpress/element';

import { SlotFillProvider } from '@wordpress/components';
/**
* Internal dependencies
*/
Expand All @@ -19,16 +19,18 @@ const noop = () => {};
function TestWrapper() {
const [ mediaURL, setMediaURL ] = useState( 'https://example.media' );
return (
<MediaReplaceFlow
mediaId={ 1 }
mediaURL={ mediaURL }
allowedTypes={ [ 'png' ] }
accept="image/*"
onSelect={ noop }
onSelectURL={ setMediaURL }
onError={ noop }
onCloseModal={ noop }
/>
<SlotFillProvider>
<MediaReplaceFlow
mediaId={ 1 }
mediaURL={ mediaURL }
allowedTypes={ [ 'png' ] }
accept="image/*"
onSelect={ noop }
onSelectURL={ setMediaURL }
onError={ noop }
onCloseModal={ noop }
/>
</SlotFillProvider>
);
}

Expand Down
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
- `SlotFill`: rewrite the non-portal version to use `observableMap` ([#67400](https://github.com/WordPress/gutenberg/pull/67400)).
- `DatePicker`: Prepare day buttons for 40px default size ([#68156](https://github.com/WordPress/gutenberg/pull/68156)).
- `SlotFill`: register slots in a layout effect ([#68176](https://github.com/WordPress/gutenberg/pull/68176)).
- `SlotFill`: unify context providers and `Fill` implementations ([#68056](https://github.com/WordPress/gutenberg/pull/68056)).

## 29.0.0 (2024-12-11)

Expand Down
50 changes: 0 additions & 50 deletions packages/components/src/slot-fill/bubbles-virtually/fill.tsx

This file was deleted.

This file was deleted.

This file was deleted.

25 changes: 16 additions & 9 deletions packages/components/src/slot-fill/bubbles-virtually/slot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { useMergeRefs } from '@wordpress/compose';
* Internal dependencies
*/
import { View } from '../../view';
import SlotFillContext from './slot-fill-context';
import SlotFillContext from '../context';
import type { WordPressComponentProps } from '../../context';
import type { SlotComponentProps } from '../types';

Expand All @@ -40,25 +40,32 @@ function Slot(
} = props;

const registry = useContext( SlotFillContext );

const instanceRef = useRef( {} );
const ref = useRef< HTMLElement >( null );

// We don't want to unregister and register the slot whenever
// `fillProps` change, which would cause the fill to be re-mounted. Instead,
// we can just update the slot (see hook below).
// For more context, see https://github.com/WordPress/gutenberg/pull/44403#discussion_r994415973
const fillPropsRef = useRef( fillProps );
useLayoutEffect( () => {
fillPropsRef.current = fillProps;
}, [ fillProps ] );

useLayoutEffect( () => {
registry.registerSlot( name, ref, fillPropsRef.current );
return () => registry.unregisterSlot( name, ref );
const instance = instanceRef.current;
registry.registerSlot( name, {
type: 'portal',
instance,
ref,
fillProps: fillPropsRef.current,
} );
return () => registry.unregisterSlot( name, instance );
}, [ registry, name ] );

useLayoutEffect( () => {
registry.updateSlot( name, ref, fillPropsRef.current );
registry.updateSlot( name, {
type: 'portal',
instance: instanceRef.current,
ref,
fillProps: fillPropsRef.current,
} );
} );

return (
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,42 @@
/**
* WordPress dependencies
*/
import { useContext } from '@wordpress/element';
import { useObservableValue } from '@wordpress/compose';
import { useContext, useMemo, useSyncExternalStore } from '@wordpress/element';
import type { ObservableMap } from '@wordpress/compose';

/**
* Internal dependencies
*/
import SlotFillContext from './slot-fill-context';
import SlotFillContext from '../context';
import type { SlotKey } from '../types';

function useObservableValueWithSelector< K, V, S >(
Copy link
Member

Choose a reason for hiding this comment

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

Is this worth living next to useObservableValue()?

map: ObservableMap< K, V >,
name: K,
selector: ( v: V | undefined ) => S
) {
const subscribe = useMemo(
() => ( listener: () => void ) => map.subscribe( name, listener ),
[ map, name ]
);
const getValue = () => selector( map.get( name ) );
return useSyncExternalStore( subscribe, getValue, getValue );
}

function getLength< T >( array: T[] | undefined ) {
return array?.length;
}

export default function useSlotFills( name: SlotKey ) {
const registry = useContext( SlotFillContext );
return useObservableValue( registry.fills, name );
const length = useObservableValueWithSelector(
registry.fills,
name,
getLength
Copy link
Member

Choose a reason for hiding this comment

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

Is observing the array length enough? Any concerns if the fills change but have the same length?

);
// callers expect an opaque array with length `length`, so create that array
const fills = useMemo( () => {
return length !== undefined ? Array.from( { length } ) : undefined;
}, [ length ] );
return fills;
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,15 @@ import { useObservableValue } from '@wordpress/compose';
/**
* Internal dependencies
*/
import SlotFillContext from './slot-fill-context';
import type { SlotKey } from '../types';
import SlotFillContext from '../context';
import type { SlotKey, SlotRef } from '../types';

export default function useSlot( name: SlotKey ) {
const registry = useContext( SlotFillContext );
const slot = useObservableValue( registry.slots, name );
return { ...slot };
let ref: SlotRef | undefined;
if ( slot && slot.type === 'portal' ) {
ref = slot.ref;
}
return { ref };
}
Loading
Loading