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 all 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
1 change: 1 addition & 0 deletions packages/components/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ export { default as ToolbarItem } from './toolbar-item';
export {
ToolsPanel as __experimentalToolsPanel,
ToolsPanelItem as __experimentalToolsPanelItem,
ToolsPanelContext as __experimentalToolsPanelContext,
} from './tools-panel';
export { default as Tooltip } from './tooltip';
export {
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/tools-panel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export { default as ToolsPanel } from './tools-panel';
export { default as ToolsPanelItem } from './tools-panel-item';
export { ToolsPanelContext } from './context';
100 changes: 94 additions & 6 deletions 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 All @@ -28,16 +29,13 @@ export const _default = () => {
const resetAll = () => {
setHeight( undefined );
setWidth( undefined );
setMinHeight( undefined );
};

return (
<PanelWrapperView>
<Panel>
<ToolsPanel
header="Tools Panel"
label="Display options"
resetAll={ resetAll }
>
<ToolsPanel label="Tools Panel" resetAll={ resetAll }>
<ToolsPanelItem
className="single-column"
hasValue={ () => !! width }
Expand Down Expand Up @@ -79,7 +77,97 @@ export const _default = () => {
);
};

const { Fill: ToolsPanelItems, Slot } = createSlotFill( 'ToolsPanelSlot' );
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 }
>
<Slot />
</ToolsPanel>
</Panel>
</PanelWrapperView>
</SlotFillProvider>
);
};

const PanelWrapperView = styled.div`
max-width: 250px;
max-width: 260px;
font-size: 13px;
`;
27 changes: 23 additions & 4 deletions packages/components/src/tools-panel/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ const resetAll = jest.fn();

// Default props for the tools panel.
const defaultProps = {
header: 'Panel header',
label: 'Display options',
label: 'Panel header',
resetAll,
};

Expand Down Expand Up @@ -231,9 +230,9 @@ describe( 'ToolsPanel', () => {
expect( menuItems[ 1 ] ).toHaveAttribute( 'aria-checked', 'false' );
} );

it( 'should render panel header', () => {
it( 'should render panel label as header text', () => {
renderPanel();
const header = screen.getByText( defaultProps.header );
const header = screen.getByText( defaultProps.label );

expect( header ).toBeInTheDocument();
} );
Expand Down Expand Up @@ -316,6 +315,26 @@ describe( 'ToolsPanel', () => {
await selectMenuItem( 'Reset all' );

expect( resetAll ).toHaveBeenCalledTimes( 1 );
expect( controlProps.onSelect ).not.toHaveBeenCalled();
expect( controlProps.onDeselect ).not.toHaveBeenCalled();
expect( altControlProps.onSelect ).not.toHaveBeenCalled();
expect( altControlProps.onDeselect ).not.toHaveBeenCalled();
} );

// This confirms the internal `isResetting` state when resetting all
// controls does not prevent subsequent individual reset requests.
// i.e. onDeselect callbacks are called correctly after a resetAll.
it( 'should call onDeselect after previous reset all', async () => {
renderPanel();

await selectMenuItem( 'Reset all' ); // Initial control is displayed by default.
await selectMenuItem( controlProps.label ); // Re-display control.

expect( controlProps.onDeselect ).not.toHaveBeenCalled();

await selectMenuItem( controlProps.label ); // Reset control.

expect( controlProps.onDeselect ).toHaveBeenCalled();
} );
} );

Expand Down
13 changes: 4 additions & 9 deletions 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 @@ -18,22 +18,17 @@ This component is generated automatically by its parent

## Props

### `header`: `string`
### `label`: `string`

Text to be displayed within the panel header.
Text to be displayed within the panel header. It is also passed along as the
`label` for the panel header's `DropdownMenu`.

- Required: Yes

### `menuLabel`: `string`

This is passed along as the `label` for the panel header's `DropdownMenu`.

- Required: No

### `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
9 changes: 4 additions & 5 deletions packages/components/src/tools-panel/tools-panel-header/component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,22 @@ import { contextConnect } from '../../ui/context';
const ToolsPanelHeader = ( props, forwardedRef ) => {
const {
hasMenuItems,
header,
label: labelText,
menuItems,
menuLabel,
resetAll,
toggleItem,
...headerProps
} = useToolsPanelHeader( props );

if ( ! header ) {
if ( ! labelText ) {
return null;
}

return (
<h2 { ...headerProps } ref={ forwardedRef }>
{ header }
{ labelText }
{ hasMenuItems && (
<DropdownMenu icon={ moreVertical } label={ menuLabel }>
<DropdownMenu icon={ moreVertical } label={ labelText }>
{ ( { onClose } ) => (
<>
<MenuGroup label={ __( 'Display options' ) }>
Expand Down
40 changes: 38 additions & 2 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 All @@ -18,6 +18,13 @@ for how to use `ToolsPanelItem`.

## Props

### `hasValue`: `function`

This is called when building the `ToolsPanel` menu to determine the item's
initial checked state.

- Required: Yes

### `isShownByDefault`: `boolean`

This prop identifies the current item as being displayed by default. This means
Expand All @@ -30,10 +37,39 @@ 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.

A panel item's `label` should be unique among all items within a single panel.

- Required: Yes

### `onDeselect`: `function`

Called when this item is deselected in the `ToolsPanel` menu. This is normally
used to reset the panel item control's value.

- Required: No

### `onSelect`: `function`

A callback to take action when this item is selected in the `ToolsPanel` menu.

- Required: No

### `panelId`: `string`

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 functions through to the panel's `resetAll` callback. They can then be
iterated over to perform additional tasks.

- Required: No
24 changes: 18 additions & 6 deletions packages/components/src/tools-panel/tools-panel-item/hook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ export function useToolsPanelItem( props ) {
hasValue,
isShownByDefault,
label,
panelId,
resetAllFilter,
onDeselect = () => undefined,
onSelect = () => undefined,
...otherProps
Expand All @@ -29,19 +31,25 @@ export function useToolsPanelItem( props ) {
} );

const {
panelId: currentPanelId,
menuItems,
registerPanelItem,
deregisterPanelItem,
isResetting,
} = useToolsPanelContext();

// Registering the panel item allows the panel to include it in its
// automatically generated menu and determine its initial checked status.
useEffect( () => {
registerPanelItem( {
hasValue,
isShownByDefault,
label,
} );
if ( currentPanelId === panelId ) {
registerPanelItem( {
hasValue,
isShownByDefault,
label,
resetAllFilter,
panelId,
} );
}

return () => deregisterPanelItem( label );
}, [] );
Expand All @@ -56,14 +64,18 @@ export function useToolsPanelItem( props ) {
// Determine if the panel item's corresponding menu is being toggled and
// trigger appropriate callback if it is.
useEffect( () => {
if ( isResetting ) {
return;
}

if ( isMenuItemChecked && ! isValueSet && ! wasMenuItemChecked ) {
onSelect();
}

if ( ! isMenuItemChecked && wasMenuItemChecked ) {
onDeselect();
}
}, [ isMenuItemChecked, wasMenuItemChecked, isValueSet ] );
}, [ isMenuItemChecked, wasMenuItemChecked, isValueSet, isResetting ] );

return {
...otherProps,
Expand Down