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

InspectorControls: Wrap block support slots in ToolsPanel #34157

Merged
merged 3 commits into from
Sep 10, 2021
Merged

InspectorControls: Wrap block support slots in ToolsPanel #34157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
269b5d1
Wrap block support slots in ToolsPanel for dimensions support
aaronrobertshaw Aug 19, 2021
240f1c7
Fix regression in ToolsPanel isResetting handling
aaronrobertshaw Sep 10, 2021
abf5b6a
Fix typo
aaronrobertshaw Sep 10, 2021
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
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/block-inspector/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@ const BlockInspectorSingleBlock = ( {
</div>
) }
<InspectorControls.Slot bubblesVirtually={ bubblesVirtually } />
<InspectorControls.Slot
__experimentalGroup="dimensions"
bubblesVirtually={ false }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use bubblesVirtuall = { false }. Ideally we always use true as we were thinking of deprecating the other approach ultimately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found when bubblesVirtually was true it interfered with the registration of panel items via the ToolsPanel's context.

Copy link
Member

Choose a reason for hiding this comment

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

It's a know limitation that you need to propagate the context when using slots that bubble virtually. An example when the context is passed down through fillProps is for the block controls:

https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/block-controls/slot.js

@diegohaz might remember better the implementation details. I'm not sure how much it differs from the use case you have, but we should try to generalize the solution if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @gziolo, the example you gave has helped get me most of the way to fixing this up.

The only real difference I see in this use case is that the ToolsPanelContext isn't being created until the ToolsPanel is when it wraps the Slot. I believe this means we need to wrap the Slot again so that component can grab the context and pass it through the fillProps.

I tried a HoC approach here but couldn't get that working. Instead, I have a regular component handling this. It almost "works" except that it introduces a rogue <div> wrapping all the fills. This breaks the layout of the ToolsPanel. I haven't been able to put my finger on where I've gone wrong here.

Any help you can offer here would be greatly appreciated 🙏

Also, would this be something I could continue to iterate on in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

If everything else is working then you can tackle it separately. It's Important to find a good solution here because the same approach is going to be replicated in other places 😄

Copy link
Member

Choose a reason for hiding this comment

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

The only real difference I see in this use case is that the ToolsPanelContext isn't being created until the ToolsPanel is when it wraps the Slot. I believe this means we need to wrap the Slot again so that component can grab the context and pass it through the fillProps.

How is the component tree structured? Won't the Slot component be rendered inside ToolsPanel, in which case it would have access to the parent context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If everything else is working then you can tackle it separately. It's Important to find a good solution here because the same approach is going to be replicated in other places 😄

This PR's current state is working well according to the most recent reviews and testing. Myself, @glendaviesnz and @andrewserong have all put it through its paces.

If you are comfortable with this being merged, including the bubblesVirtually={ false }, I'll definitely continue iterating on this separately until that good solution is found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit out of my normal wheelhouse so please excuse my lack of understanding around this.

How is the component tree structured? Won't the Slot component be rendered inside ToolsPanel, in which case it would have access to the parent context?

That was what I was originally expecting but it only worked when preventing virtual bubbling.

The ToolsPanel creates the context and wraps its children in the context provider. The slot is rendered within that. The panel items are added via that slot. Those panel items need to access the ToolsPanelContext to register themselves with the panel.

My current understanding is that the slot will have access to the context but the context has to be passed through fillProps so that we can re-create the context provider to wrap the fills thereby allowing the panel items access to the context again.

I hope that makes sense!

label={ __( 'Dimensions' ) }
/>
<div>
<AdvancedControls bubblesVirtually={ bubblesVirtually } />
</div>
Expand Down
58 changes: 58 additions & 0 deletions packages/block-editor/src/components/inspector-controls/block-support-tools-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
* WordPress dependencies
*/
import { __experimentalToolsPanel as ToolsPanel } from '@wordpress/components';
import { useDispatch, useSelect } from '@wordpress/data';

/**
* Internal dependencies
*/
import { store as blockEditorStore } from '../../store';
import { cleanEmptyObject } from '../../hooks/utils';

export default function BlockSupportToolsPanel( { children, label, header } ) {
const { clientId, attributes } = useSelect( ( select ) => {
const { getBlockAttributes, getSelectedBlockClientId } = select(
blockEditorStore
);
const selectedBlockClientId = getSelectedBlockClientId();
Copy link
Member

Choose a reason for hiding this comment

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

We are exploring ways to absorb parent controls as proposed in #26313. The fact that this panel depends on the currently selected block would prevent it from being usable in such context. Maybe it's fine because we might never get to the point where you could expose those controls from the parent to child blocks. It might be also not hard to refactor it when needed and pass the parent client id through the slot when necessary. I mostly wanted to raise awareness. I don't think we need to worry that much about it in the initial implementation, but we should keep that in mind that this panel doesn't necessarily need to be concerned only about the currently selected block.


return {
clientId: selectedBlockClientId,
attributes: getBlockAttributes( selectedBlockClientId ),
};
} );
const { updateBlockAttributes } = useDispatch( blockEditorStore );

const resetAll = ( resetFilters = [] ) => {
const { style } = attributes;
let newAttributes = { style };

resetFilters.forEach( ( resetFilter ) => {
newAttributes = {
...newAttributes,
...resetFilter( newAttributes ),
};
} );

// Enforce a cleaned style object.
newAttributes = {
...newAttributes,
style: cleanEmptyObject( newAttributes.style ),
};

updateBlockAttributes( clientId, newAttributes );
};

return (
<ToolsPanel
label={ label }
header={ header }
resetAll={ resetAll }
key={ clientId }
panelId={ clientId }
>
{ children }
</ToolsPanel>
);
}
4 changes: 4 additions & 0 deletions packages/block-editor/src/components/inspector-controls/groups.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@ import { createSlotFill } from '@wordpress/components';

const InspectorControlsDefault = createSlotFill( 'InspectorControls' );
const InspectorControlsAdvanced = createSlotFill( 'InspectorAdvancedControls' );
const InspectorControlsDimensions = createSlotFill(
'InspectorControlsDimensions'
);

const groups = {
default: InspectorControlsDefault,
advanced: InspectorControlsAdvanced,
dimensions: InspectorControlsDimensions,
};

export default groups;
10 changes: 10 additions & 0 deletions packages/block-editor/src/components/inspector-controls/slot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ import warning from '@wordpress/warning';
/**
* Internal dependencies
*/
import BlockSupportToolsPanel from './block-support-tools-panel';
import groups from './groups';

export default function InspectorControlsSlot( {
__experimentalGroup: group = 'default',
bubblesVirtually = true,
label,
...props
} ) {
const Slot = groups[ group ]?.Slot;
Expand All @@ -26,5 +28,13 @@ export default function InspectorControlsSlot( {
return null;
}

if ( label ) {
return (
<BlockSupportToolsPanel group={ group } label={ label }>
<Slot { ...props } bubblesVirtually={ bubblesVirtually } />
</BlockSupportToolsPanel>
);
}

return <Slot { ...props } bubblesVirtually={ bubblesVirtually } />;
}
108 changes: 49 additions & 59 deletions packages/block-editor/src/hooks/dimensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
/**
* WordPress dependencies
*/
import {
__experimentalToolsPanel as ToolsPanel,
__experimentalToolsPanelItem as ToolsPanelItem,
} from '@wordpress/components';
import { __experimentalToolsPanelItem as ToolsPanelItem } from '@wordpress/components';
import { Platform } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { getBlockSupport } from '@wordpress/blocks';
Expand Down Expand Up @@ -34,7 +31,6 @@ import {
resetPadding,
useIsPaddingDisabled,
} from './padding';
import { cleanEmptyObject } from './utils';

export const SPACING_SUPPORT_KEY = 'spacing';
export const ALL_SIDES = [ 'top', 'right', 'bottom', 'left' ];
Expand Down Expand Up @@ -63,62 +59,56 @@ export function DimensionsPanel( props ) {
'__experimentalDefaultControls',
] );

// Callback to reset all block support attributes controlled via this panel.
const resetAll = () => {
const { style } = props.attributes;

props.setAttributes( {
style: cleanEmptyObject( {
...style,
spacing: {
...style?.spacing,
blockGap: undefined,
margin: undefined,
padding: undefined,
},
} ),
} );
};
const createResetAllFilter = ( attribute ) => ( newAttributes ) => ( {
...newAttributes,
style: {
...newAttributes.style,
spacing: {
...newAttributes.style?.spacing,
[ attribute ]: undefined,
},
},
} );

return (
<InspectorControls key="dimensions">
<ToolsPanel
label={ __( 'Dimensions options' ) }
header={ __( 'Dimensions' ) }
resetAll={ resetAll }
>
{ ! isPaddingDisabled && (
<ToolsPanelItem
hasValue={ () => hasPaddingValue( props ) }
label={ __( 'Padding' ) }
onDeselect={ () => resetPadding( props ) }
isShownByDefault={ defaultSpacingControls?.padding }
>
<PaddingEdit { ...props } />
</ToolsPanelItem>
) }
{ ! isMarginDisabled && (
<ToolsPanelItem
hasValue={ () => hasMarginValue( props ) }
label={ __( 'Margin' ) }
onDeselect={ () => resetMargin( props ) }
isShownByDefault={ defaultSpacingControls?.margin }
>
<MarginEdit { ...props } />
</ToolsPanelItem>
) }
{ ! isGapDisabled && (
<ToolsPanelItem
className="single-column"
hasValue={ () => hasGapValue( props ) }
label={ __( 'Block gap' ) }
onDeselect={ () => resetGap( props ) }
isShownByDefault={ defaultSpacingControls?.blockGap }
>
<GapEdit { ...props } />
</ToolsPanelItem>
) }
</ToolsPanel>
<InspectorControls __experimentalGroup="dimensions">
{ ! isPaddingDisabled && (
<ToolsPanelItem
hasValue={ () => hasPaddingValue( props ) }
label={ __( 'Padding' ) }
onDeselect={ () => resetPadding( props ) }
resetAllFilter={ createResetAllFilter( 'padding' ) }
isShownByDefault={ defaultSpacingControls?.padding }
panelId={ props.clientId }
>
<PaddingEdit { ...props } />
</ToolsPanelItem>
) }
{ ! isMarginDisabled && (
<ToolsPanelItem
hasValue={ () => hasMarginValue( props ) }
label={ __( 'Margin' ) }
onDeselect={ () => resetMargin( props ) }
resetAllFilter={ createResetAllFilter( 'margin' ) }
isShownByDefault={ defaultSpacingControls?.margin }
panelId={ props.clientId }
>
<MarginEdit { ...props } />
</ToolsPanelItem>
) }
{ ! isGapDisabled && (
<ToolsPanelItem
className="single-column"
hasValue={ () => hasGapValue( props ) }
label={ __( 'Block gap' ) }
onDeselect={ () => resetGap( props ) }
resetAllFilter={ createResetAllFilter( 'blockGap' ) }
isShownByDefault={ defaultSpacingControls?.blockGap }
panelId={ props.clientId }
>
<GapEdit { ...props } />
</ToolsPanelItem>
) }
</InspectorControls>
);
}
Expand Down
17 changes: 11 additions & 6 deletions packages/components/src/tools-panel/tools-panel/hook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,17 @@ export function useToolsPanel( props ) {
}, [ className ] );

const isResetting = useRef( false );
const wasResetting = isResetting.current;

// `isResetting` is cleared via this hook to effectively batch together
// the resetAll task. Without this, the flag is cleared after the first
// control updates and forces a rerender with subsequent controls then
// believing they need to reset, unfortunately using stale data.
useEffect( () => {
if ( wasResetting ) {
isResetting.current = false;
}
}, [ wasResetting ] );

// Allow panel items to register themselves.
const [ panelItems, setPanelItems ] = useState( [] );
Expand Down Expand Up @@ -104,12 +115,6 @@ export function useToolsPanel( props ) {
isResetting: isResetting.current,
};

// Clean up isResetting after advising panel context we were resetting
// all controls. This lets panel items know to skip onDeselect callbacks.
if ( isResetting.current ) {
isResetting.current = false;
}

return {
...otherProps,
panelContext,
Expand Down
6 changes: 1 addition & 5 deletions packages/edit-site/src/components/sidebar/dimensions-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,7 @@ export default function DimensionsPanel( { context, getStyle, setStyle } ) {
};

return (
<ToolsPanel
label={ __( 'Dimensions options' ) }
header={ __( 'Dimensions' ) }
resetAll={ resetAll }
>
<ToolsPanel label={ __( 'Dimensions' ) } resetAll={ resetAll }>
{ showPaddingControl && (
<ToolsPanelItem
hasValue={ hasPaddingValue }
Expand Down