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.

Sign up for GitHub

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

Block Controls SlotFill: refactor to allow passing multiple contexts, including internal components context #51264

Merged
merged 5 commits into from
Jun 7, 2023
Merged

Block Controls SlotFill: refactor to allow passing multiple contexts, including internal components context #51264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
86f704b
SlotFill: pass array of contexts in fillProps, use it to pass compone…
jsnajdr Jun 5, 2023
713c6db
Use lock/unlock instead of exporting experimental API
ciampo Jun 6, 2023
17abb77
Pass props object instead of just value prop
jsnajdr Jun 6, 2023
d3d28b2
Mobile BlockControlsSlot: remove ToolbarContext teleport, the context…
jsnajdr Jun 7, 2023
98ad917
Put forwardedContext into a fillProps field
jsnajdr Jun 7, 2023
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
30 changes: 15 additions & 15 deletions packages/block-editor/src/components/block-controls/fill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import {
__experimentalStyleProvider as StyleProvider,
__experimentalToolbarContext as ToolbarContext,
ToolbarGroup,
} from '@wordpress/components';

Expand All @@ -26,24 +25,25 @@ export default function BlockControlsFill( {
return null;
}

const innerMarkup = (
<>
{ group === 'default' && <ToolbarGroup controls={ controls } /> }
{ children }
</>
);

return (
<StyleProvider document={ document }>
<Fill>
{ ( fillProps ) => {
// Children passed to BlockControlsFill will not have access to any
// React Context whose Provider is part of the BlockControlsSlot tree.
// So we re-create the Provider in this subtree.
const value =
fillProps && Object.keys( fillProps ).length > 0
? fillProps
: null;
return (
<ToolbarContext.Provider value={ value }>
{ group === 'default' && (
<ToolbarGroup controls={ controls } />
) }
{ children }
</ToolbarContext.Provider>
// `fillProps.forwardedContext` is an array of context provider entries, provided by slot,
// that should wrap the fill markup.
const { forwardedContext = [] } = fillProps;
return forwardedContext.reduce(
( inner, [ Provider, props ] ) => (
<Provider { ...props }>{ inner }</Provider>
),
innerMarkup
);
} }
</Fill>
Expand Down
39 changes: 21 additions & 18 deletions packages/block-editor/src/components/block-controls/slot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
/**
* WordPress dependencies
*/
import { useContext } from '@wordpress/element';
import { useContext, useMemo } from '@wordpress/element';
import {
privateApis,
__experimentalToolbarContext as ToolbarContext,
ToolbarGroup,
__experimentalUseSlotFills as useSlotFills,
Expand All @@ -13,9 +14,23 @@ import warning from '@wordpress/warning';
* Internal dependencies
*/
import groups from './groups';
import { unlock } from '../../lock-unlock';

const { ComponentsContext } = unlock( privateApis );

export default function BlockControlsSlot( { group = 'default', ...props } ) {
const accessibleToolbarState = useContext( ToolbarContext );
const toolbarState = useContext( ToolbarContext );
const contextState = useContext( ComponentsContext );
const fillProps = useMemo(
() => ( {
forwardedContext: [
[ ToolbarContext.Provider, { value: toolbarState } ],
[ ComponentsContext.Provider, { value: contextState } ],
],
} ),
[ toolbarState, contextState ]
);

const Slot = groups[ group ]?.Slot;
const fills = useSlotFills( Slot?.__unstableName );
if ( ! Slot ) {
Expand All @@ -27,23 +42,11 @@ export default function BlockControlsSlot( { group = 'default', ...props } ) {
return null;
}

const slot = <Slot { ...props } bubblesVirtually fillProps={ fillProps } />;

if ( group === 'default' ) {
return (
<Slot
{ ...props }
bubblesVirtually
fillProps={ accessibleToolbarState }
/>
);
return slot;
}

return (
<ToolbarGroup>
<Slot
{ ...props }
bubblesVirtually
fillProps={ accessibleToolbarState }
/>
</ToolbarGroup>
);
return <ToolbarGroup>{ slot }</ToolbarGroup>;
}
11 changes: 3 additions & 8 deletions packages/block-editor/src/components/block-controls/slot.native.js
View file
Open in desktop
Copy link
Contributor Author

@ciampo ciampo Jun 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also cc'ing @dcalhoun and @fluiddot since this PR touches native files, although the code changes are just a refactor of how data is passed to the Slot

Copy link
Member

Choose a reason for hiding this comment

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

Do we also use the components context system in React Native code? Because I added the provider teleporting only to the web BlockControlsSlot, not to the mobile one.

Copy link
Member

Choose a reason for hiding this comment

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

Also it seems that in React Native we don't support the bubblesVirtually variant with portals. The bubblesVirtually prop is simply ignored. The Fill is always rendered in the Slot's React tree. It doesn't see context from the Fill tree, and teleporting the toolbar context is not needed, the component already sees it.

Copy link
Contributor Author

@ciampo ciampo Jun 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Folks from the mobile team may be best suited to answer here.

From a quick look, I don't think that there are native components explicitly using the components context system, although it is my understanding that web versions of components could be used by React Native, meaning that we'll likely neet to pass all the context values that we do in the web counterpart?

Also it seems that in React Native we don't support the bubblesVirtually variant with portals. The bubblesVirtually prop is simply ignored. The Fill is always rendered in the Slot's React tree. It doesn't see context from the Fill tree, and teleporting the toolbar context is not needed, the component already sees it.

If that's the case, though, then forwarding such contexts is not necessary, as you say.

Copy link
Member

Choose a reason for hiding this comment

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

I pushed a commit that removes context forwarding (teleporting) from the slot.native.js component completely, including the pre-exisiting ToolbarContext. Mobile uses the non-virtual Slot implementation, and all these contexts (toolbar, component system, Emotion styles) are already present it its parent React tree.

We'll need to reintroduce the forwarding when we migrate mobile slotfills to the portal bubblesVirtually implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the call out @ciampo 🙇 ! As you shared in the above comments, the SlotFill implementation in React Native is limited compared to the web version as we don't support bubblesVirtually. I checked the usage of ToolbarContext and seems we are not using it, only in the web version, so I agree on removing it from here.

Web:

gutenberg/packages/components/src/toolbar/toolbar/toolbar-container.tsx

Line 35 in e892b05

<ToolbarContext.Provider value={ toolbarState }>

Native:

gutenberg/packages/components/src/toolbar/toolbar/toolbar-container.native.js

Line 6 in e892b05

const ToolbarContainer = ( { children } ) => <View>{ children }</View>;

Regarding these changes, I tested locally and I encountered an error that I shared here. Let me know if I can help anyhow, thanks!

Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
/**
* WordPress dependencies
*/
import { useContext } from '@wordpress/element';
import {
__experimentalToolbarContext as ToolbarContext,
ToolbarGroup,
} from '@wordpress/components';
import { ToolbarGroup } from '@wordpress/components';
import warning from '@wordpress/warning';

/**
Expand All @@ -14,19 +10,18 @@ import warning from '@wordpress/warning';
import groups from './groups';

export default function BlockControlsSlot( { group = 'default', ...props } ) {
const accessibleToolbarState = useContext( ToolbarContext );
const Slot = groups[ group ]?.Slot;
if ( ! Slot ) {
warning( `Unknown BlockControls group "${ group }" provided.` );
return null;
}

if ( group === 'default' ) {
return <Slot { ...props } fillProps={ accessibleToolbarState } />;
return <Slot { ...props } />;
}

return (
<Slot { ...props } fillProps={ accessibleToolbarState }>
<Slot { ...props }>
fluiddot marked this conversation as resolved.
Show resolved Hide resolved
{ ( fills ) => {
if ( ! fills.length ) {
return null;
Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/private-apis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
DropdownSubMenu as DropdownSubMenuV2,
DropdownSubMenuTrigger as DropdownSubMenuTriggerV2,
} from './dropdown-menu-v2';
import { ComponentsContext } from './ui/context/context-system-provider';

export const { lock, unlock } =
__dangerousOptInToUnstableAPIsOnlyForCoreModules(
Expand All @@ -33,6 +34,7 @@ lock( privateApis, {
CustomSelectControl,
__experimentalPopoverLegacyPositionToPlacement,
createPrivateSlotFill,
ComponentsContext,
DropdownMenuV2,
DropdownMenuCheckboxItemV2,
DropdownMenuGroupV2,
Expand Down