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

Jump to bottom

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 4 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
31 changes: 15 additions & 16 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,24 @@ 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 = [] ) => {
// `fillProps` is an array of context provider entries, provided by slot,
// that should wrap the fill markup.
return fillProps.reduce(
( inner, [ Provider, props ] ) => (
<Provider { ...props }>{ inner }</Provider>
),
innerMarkup
);
} }
</Fill>
Expand Down
37 changes: 19 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,21 @@ 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(
() => [
[ ToolbarContext.Provider, { value: toolbarState } ],
[ ComponentsContext.Provider, { value: contextState } ],
Copy link
Member

Choose a reason for hiding this comment

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

Here I did a little change, passing a props object instead of a value for a hard-coded value prop. That makes the system more flexible.

We'll want to migrate the <StyleProvider document={...}> usage also to this format, and there's a document prop instead of value.

It would be nice to remove the hard-coded StyleProvider from the Fill implementation, as added in #38237, and configure the slots and fills that really need this explicitly.

@youknowriad added StyleProvider wrappers around specific fills in #31073. There should be eventually migrated to the fillProps format.

The StyleProviders from #31073 (specific places) and #38237 (universal for every <Fill bubblesVirtually>) often overlap each other, rendering the same provider two times next to each other. I believe the universal one should eventually go away.

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.

Thank you for the extra context, Jarda! Your suggestion makes sense in terms of code hygiene and separation of concerns, although there are a few aspects that we should also take into account:

  1. the emotion library (and therefore StyleProvider) was always meant to be kept as an internal implementation detail to the package. Adopting your suggested approach would require exporting StyleProvider from the package. Although maybe we can still keep it as a "generic" style provider (not necessarily Emotion-themed), and if one day we migrate away from Emotion that the provider is not needed anymore, we can leave it around as an empty, no-op context
  2. a good portion of @wordpress/components components would need StyleProvider to render correctly, meaning that most probably every usage of Slot would need to pass StyleProvider
  3. related to the point above, removing StyleProvider from the Fill implementation has the potential to cause visual breaking changes to third-party consumers

Copy link
Contributor

Choose a reason for hiding this comment

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

What is ComponentsContext? Can ToolbarContext be part of it?

Copy link
Member

Choose a reason for hiding this comment

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

ComponentsContext is a kind of a generic key-value store, where a parent component can tell child components things like "inside toolbar dropdown menus should be in this toolbar-specific style" or "inside input control the prefix subcomponent should have a certain padding-left". Originally comes from Q and his G2 system.

ToolbarContext is a thing coming from Reakit, where a parent component (container) exposes some API for children (items).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, so ToolbarContext is basically the same thing as ComponentsContext but specific to Toolbar components. I wonder if there's a way to unify.

],
[ toolbarState, contextState ]
);

const Slot = groups[ group ]?.Slot;
const fills = useSlotFills( Slot?.__unstableName );
if ( ! Slot ) {
Expand All @@ -27,23 +40,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