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

ToolsPanel: Allow SlotFill injection of panel items #34632

Merged
merged 6 commits into from
Sep 10, 2021
Merged

ToolsPanel: Allow SlotFill injection of panel items #34632

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
122484c
Update ToolsPanel to allow SlotFill injection of panel items
aaronrobertshaw Sep 8, 2021
0ba8d18
Fix clearing of isResetting ref
aaronrobertshaw Sep 9, 2021
4584518
Add SlotFill demo for ToolsPanel story
aaronrobertshaw Sep 9, 2021
ee6b682
Improve ToolsPanel readmes
aaronrobertshaw Sep 9, 2021
9d9aa4c
Simplify handling of SlotFill ToolsPanel example
aaronrobertshaw Sep 9, 2021
f10a027
Improve label prop description
aaronrobertshaw Sep 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
99 changes: 98 additions & 1 deletion packages/components/src/tools-panel/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { useState } from '@wordpress/element';
import { ToolsPanel, ToolsPanelItem } from '../';
import Panel from '../../panel';
import UnitControl from '../../unit-control';
import { createSlotFill, Provider as SlotFillProvider } from '../../slot-fill';

export default {
title: 'Components (Experimental)/ToolsPanel',
Expand Down Expand Up @@ -76,7 +77,103 @@ export const _default = () => {
);
};

const { Fill, Slot } = createSlotFill( 'ToolsPanelSlot' );

const ToolsPanelItems = ( { children } ) => {
return <Fill>{ children }</Fill>;
};
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
ToolsPanelItems.Slot = Slot;

const panelId = 'unique-tools-panel-id';

export const WithSlotFillItems = () => {
const [ attributes, setAttributes ] = useState( {} );
const { width, height } = attributes;

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

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

setAttributes( newAttributes );
};

const updateAttribute = ( name, value ) => {
setAttributes( {
...attributes,
[ name ]: value,
} );
};

return (
<SlotFillProvider>
<ToolsPanelItems>
Copy link
Member

@gziolo gziolo Sep 9, 2021 •
edited

Choose a reason for hiding this comment

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

This example and the documentation efforts made me think whether panelId should be part of the component. Should we create an opinionated Fill and Slot and keep them in the block editor instead?

The difference would be that ToolsPanelItem from the @wordpress/block-editor package would be a fill that handles all the logic related to panelId and renders ToolsPanelItem. There would be also a slot that would pass down panelId to fills through fillProps and would be wrapped with ToolPanel component. I don't know if I'm doing a good job explaining it.

So the goal would be to achieve the following:

const { Fill, Slot } = createSlotFill( 'ToolsPanelSlot' );

function shouldRender( panelId ) {
    // checks somehow whether to render the panel based on the value passed from slot using `fillProps`
}

const ToolsPanelItemFill = ( { panelId, ...props } ) => {
    return (
        <Fill>
            { shouldRender( panelId ) && <ToolsPanelItem { ...props } /> }
        </Fill>
    );
};

const ToolsPanelSlot = ( { panelId, ...props } ) => {
    return (
        <ToolsPanel { ...props }>
            <Slot fillProps={ panelId } />
        </ToolsPanel>
    );
};

It needs more code, but I hope it helps to sketch the idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an interesting idea. I don't think I fully understand the implications yet but will explore this tomorrow.

It makes sense in terms of being able to remove the need for panelId on the ToolsPanel itself. At this stage though, I don't have much of an idea how the individual ToolsPanelItems would be working.

Copy link
Member

Choose a reason for hiding this comment

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

You probably can play with the Storybook story first and see if you can make it work this way. As noted in another place, given that both updated components are marked as experimental don't treat my comment as a blocker.

<ToolsPanelItem
className="single-column"
hasValue={ () => !! width }
label="Injected Width"
onDeselect={ () => updateAttribute( 'width', undefined ) }
resetAllFilter={ () => ( { width: undefined } ) }
panelId={ panelId }
>
<UnitControl
label="Injected Width"
value={ width }
onChange={ ( next ) =>
updateAttribute( 'width', next )
}
/>
</ToolsPanelItem>
<ToolsPanelItem
className="single-column"
hasValue={ () => !! height }
label="Injected Height"
onDeselect={ () => updateAttribute( 'height', undefined ) }
resetAllFilter={ () => ( { height: undefined } ) }
panelId={ panelId }
>
<UnitControl
label="Injected Height"
value={ height }
onChange={ ( next ) =>
updateAttribute( 'height', next )
}
/>
</ToolsPanelItem>
<ToolsPanelItem
hasValue={ () => true }
label="Item for alternate panel"
onDeselect={ () => undefined }
resetAllFilter={ () => undefined }
panelId={ 'intended-for-another-panel-via-shared-slot' }
>
<p>
This panel item will not be displayed in the demo as its
panelId does not match the panel being rendered.
</p>
</ToolsPanelItem>
</ToolsPanelItems>
<PanelWrapperView>
<Panel>
<ToolsPanel
label="Tools Panel With SlotFill Items"
resetAll={ resetAll }
panelId={ panelId }
>
<ToolsPanelItems.Slot />
</ToolsPanel>
</Panel>
</PanelWrapperView>
</SlotFillProvider>
);
};

const PanelWrapperView = styled.div`
max-width: 250px;
max-width: 260px;
font-size: 13px;
`;
2 changes: 1 addition & 1 deletion packages/components/src/tools-panel/tools-panel-header/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ Text to be displayed within the panel header. It is also passed along as the
### `resetAll`: `function`

The `resetAll` prop provides the callback to execute when the "Reset all" menu
item is selected. It's purpose is to facilitate resetting any control values
item is selected. Its purpose is to facilitate resetting any control values
for items contained within this header's panel.

- Required: Yes
Expand Down
15 changes: 7 additions & 8 deletions packages/components/src/tools-panel/tools-panel-item/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ implementation subject to drastic and breaking changes.
</div>
<br />

This component acts a wrapper and controls the display of items to be contained
This component acts as a wrapper and controls the display of items to be contained
within a ToolsPanel. An item is displayed if it is flagged as a default control
or the corresponding panel menu item, provided via context, is toggled on for
this item.
Expand Down Expand Up @@ -37,7 +37,7 @@ panel's menu.

The supplied label is dual purpose.
It is used as:
1. the human readable label for the panel's dropdown menu
1. the human-readable label for the panel's dropdown menu
2. a key to locate the corresponding item in the panel's menu context to
determine if the panel item should be displayed.

Expand All @@ -60,17 +60,16 @@ A callback to take action when this item is selected in the `ToolsPanel` menu.

### `panelId`: `string`

This prop can be set to a ID representing a unique `ToolsPanel`. Before
attempting to register with a panel, each item will ensure that it belongs to
the current panel. This avoids issues when sharing SlotFills to inject items
into a panel.
Panel items will ensure they are only registering with their intended panel by
comparing the `panelId` props set on both the item and the panel itself. This
allows items to be injected from a shared source.

- Required: No

### `resetAllFilter`: `function`

A `ToolsPanel` will collect each item's `resetAllFilter` and pass an array of
these function through to the panel's `resetAll` callback. They can then be
iterated over to perform additional tasks for items injected via SlotFills.
these functions through to the panel's `resetAll` callback. They can then be
iterated over to perform additional tasks.

- Required: No
8 changes: 4 additions & 4 deletions packages/components/src/tools-panel/tools-panel/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,16 @@ export function DimensionPanel( props ) {

### `label`: `string`

Text to be displayed within the panel's header and as the aria label for the
Text to be displayed within the panel's header and as the aria-label for the
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
panel's dropdown menu.

- Required: Yes

### `panelId`: `function`
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved

The `panelId` is passed through the `ToolsPanelContext` to panel items. This is
be used to ensure items injected via SlotFills are only registered for their
intended panels.
If a `panelId` is set, it is passed through the `ToolsPanelContext` and used
to restrict panel items. Only items with a matching `panelId` will be able
to register themselves with this panel.

- Required: No

Expand Down