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 Supports: Switch dimensions inspector controls slot to bubble virtually #34725

Merged
merged 16 commits into from
Oct 8, 2021
Merged

Block Supports: Switch dimensions inspector controls slot to bubble virtually #34725

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
220911c
Switch dimensions inspector controls slot to bubble virtually
aaronrobertshaw Sep 10, 2021
326d3d2
Move creation of ToolsPanel to within fills
aaronrobertshaw Sep 13, 2021
8d238f7
Revert "Move creation of ToolsPanel to within fills"
aaronrobertshaw Sep 13, 2021
72d556e
Remove unused header prop
aaronrobertshaw Sep 13, 2021
9b09362
Fix layout when slot injects wrapping div inside tools panel
aaronrobertshaw Sep 14, 2021
8507363
Add optional placeholder rendering to maintain order of fills
aaronrobertshaw Sep 22, 2021
e5809a8
Use props to setup handling of wrapper div and empty styles
aaronrobertshaw Sep 23, 2021
5d4677c
Replace is-empty tests with ones specific to item order
aaronrobertshaw Sep 24, 2021
0749b9b
Add `hasInnerWrapper` to ToolsPanelProps type
aaronrobertshaw Sep 24, 2021
84ba3b8
Add new props to ToolsPanel README
aaronrobertshaw Sep 24, 2021
6d54a19
Fix description of hasInnerWrapper prop
aaronrobertshaw Oct 5, 2021
b1b93b5
Improve shouldRenderPlaceholderItems description
aaronrobertshaw Oct 5, 2021
a148041
Fix comment regarding tracking optional menu items
aaronrobertshaw Oct 5, 2021
540bcb3
Move classname computations into useMemo hooks
aaronrobertshaw Oct 6, 2021
ce2ef0a
Refactor shared grid layout styles
aaronrobertshaw Oct 6, 2021
022c19f
Tweak cleanup for controlProps in tests
aaronrobertshaw Oct 6, 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
2 changes: 1 addition & 1 deletion 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 @@ -131,7 +131,7 @@ const BlockInspectorSingleBlock = ( {
<InspectorControls.Slot bubblesVirtually={ bubblesVirtually } />
<InspectorControls.Slot
__experimentalGroup="dimensions"
bubblesVirtually={ false }
bubblesVirtually={ bubblesVirtually }
label={ __( 'Dimensions' ) }
/>
<div>
Expand Down
10 changes: 10 additions & 0 deletions packages/block-editor/src/components/inspector-controls/block-support-slot-container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* WordPress dependencies
*/
import { __experimentalToolsPanelContext as ToolsPanelContext } from '@wordpress/components';
import { useContext } from '@wordpress/element';

export default function BlockSupportSlotContainer( { Slot, ...props } ) {
const toolsPanelContext = useContext( ToolsPanelContext );
return <Slot { ...props } fillProps={ toolsPanelContext } />;
}
5 changes: 3 additions & 2 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
Expand Up @@ -10,7 +10,7 @@ import { useDispatch, useSelect } from '@wordpress/data';
import { store as blockEditorStore } from '../../store';
import { cleanEmptyObject } from '../../hooks/utils';

export default function BlockSupportToolsPanel( { children, label, header } ) {
export default function BlockSupportToolsPanel( { children, label } ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The header prop is no longer used. I missed this when separating the ToolsPanel changes from those adding the ToolsPanel and Dimensions slot in #34157

const { clientId, attributes } = useSelect( ( select ) => {
const { getBlockAttributes, getSelectedBlockClientId } = select(
blockEditorStore
Expand Down Expand Up @@ -47,10 +47,11 @@ export default function BlockSupportToolsPanel( { children, label, header } ) {
return (
<ToolsPanel
label={ label }
header={ header }
resetAll={ resetAll }
key={ clientId }
panelId={ clientId }
hasInnerWrapper={ true }
shouldRenderPlaceholderItems={ true } // Required to maintain fills ordering.
>
{ children }
</ToolsPanel>
Expand Down
25 changes: 23 additions & 2 deletions packages/block-editor/src/components/inspector-controls/fill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
/**
* External dependencies
*/
import { isEmpty } from 'lodash';

/**
* WordPress dependencies
*/
import { __experimentalStyleProvider as StyleProvider } from '@wordpress/components';
import {
__experimentalStyleProvider as StyleProvider,
__experimentalToolsPanelContext as ToolsPanelContext,
} from '@wordpress/components';
import warning from '@wordpress/warning';

/**
Expand All @@ -26,7 +34,20 @@ export default function InspectorControlsFill( {

return (
<StyleProvider document={ document }>
<Fill>{ children }</Fill>
<Fill>
{ ( fillProps ) => {
// Children passed to InspectorControlsFill will not have
// access to any React Context whose Provider is part of
// the InspectorControlsSlot tree. So we re-create the
// Provider in this subtree.
const value = ! isEmpty( fillProps ) ? fillProps : null;
return (
<ToolsPanelContext.Provider value={ value }>
{ children }
</ToolsPanelContext.Provider>
);
} }
</Fill>
</StyleProvider>
);
}
7 changes: 6 additions & 1 deletion 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 @@ -8,6 +8,7 @@ import warning from '@wordpress/warning';
* Internal dependencies
*/
import BlockSupportToolsPanel from './block-support-tools-panel';
import BlockSupportSlotContainer from './block-support-slot-container';
import groups from './groups';

export default function InspectorControlsSlot( {
Expand All @@ -31,7 +32,11 @@ export default function InspectorControlsSlot( {
if ( label ) {
return (
<BlockSupportToolsPanel group={ group } label={ label }>
<Slot { ...props } bubblesVirtually={ bubblesVirtually } />
<BlockSupportSlotContainer
Copy link
Member

Choose a reason for hiding this comment

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

Does BlockSupportSlotContainer exist only to collocate the context hook with the Slot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point in time that's correct. I didn't find a better option given the tools panel context isn't created until the ToolsPanel is within the BlockSupportToolsPanel component.

With the current approach, there is also an issue with maintaining the order of rendered items in the panel when they are injected from multiple places. I'm definitely open to suggestions on improving this.

Another option I was planning to explore but expect some issues with was, to see if I could set up ref forwarding for the BlockSupportToolsPanel and pass it through the as prop to the virtual bubbling Slot. The idea being that the BlockSupportToolsPanel then passes the slot's ref on to a wrapping div and then creates the ToolsPanel with the fills inside that. The goal being the slot no longer introduces a div inside the ToolsPanel and there would no longer be a need to manually pass the ToolsPanelContext through the slot.

As I mentioned, I'm probably missing something with that idea. Let me know if there are any fatal flaws with it.

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 haven't had much luck trying to find a working solution to avoid this or the fact the virtual bubbling slot introduces a div when it would be preferential to avoid that inside the ToolsPanel.

One problem I ran into was that the virtual bubbling slots do not appear to accept a function as a child as per the normal slot component.

Slot also accepts optional children function prop, which takes fills as a param. It allows to perform additional processing and wrap fills conditionally.

The docs do not appear to make any distinction here. @gziolo or @youknowriad can you confirm if this is an issue with the docs or whether the virtual bubbling slot component should also offer that?

While exploring this, it looked like it might be possible to leverage the fills retrieved within the inspector controls slot and do something with them from there but that felt like the wrong path to go down. As things stand now, this PR gets the block support tools panel and slots functional. We can iterate on this in follow up PRs.

{ ...props }
bubblesVirtually={ bubblesVirtually }
Slot={ Slot }
/>
</BlockSupportToolsPanel>
);
}
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/tools-panel/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export const ToolsPanelContext = createContext< ToolsPanelContextType >( {
menuItems: { default: {}, optional: {} },
hasMenuItems: false,
isResetting: false,
shouldRenderPlaceholderItems: false,
registerPanelItem: noop,
deregisterPanelItem: noop,
flagItemCustomization: noop,
Expand Down
33 changes: 33 additions & 0 deletions packages/components/src/tools-panel/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,27 @@ export const ToolsPanel = css`
row-gap: ${ space( 6 ) };
`;

/**
* Items injected into a ToolsPanel via a virtual bubbling slot will require
* an inner dom element to be injected. The following rule allows for the
* CSS grid display to be re-established.
*/
export const ToolsPanelWithInnerWrapper = css`
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
> div {
column-gap: ${ space( 4 ) };
display: grid;
grid-template-columns: 1fr 1fr;
row-gap: ${ space( 6 ) };
grid-column: span 2;
}
`;

export const ToolsPanelHiddenInnerWrapper = css`
> div {
display: none;
}
`;

export const ToolsPanelHeader = css`
align-items: center;
display: flex;
Expand Down Expand Up @@ -61,6 +82,18 @@ export const ToolsPanelItem = css`
margin-bottom: 0;
max-width: 100%;
}

& > .components-base-control:last-child {
margin-bottom: 0;

.components-base-control__field {
margin-bottom: 0;
}
}
`;

export const ToolsPanelItemPlaceholder = css`
display: none;
`;

export const DropdownMenu = css`
Expand Down
86 changes: 86 additions & 0 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 @@ -7,7 +7,9 @@ import { render, screen, fireEvent } from '@testing-library/react';
* Internal dependencies
*/
import { ToolsPanel, ToolsPanelItem } from '../';
import { createSlotFill, Provider as SlotFillProvider } from '../../slot-fill';

const { Fill: ToolsPanelItems, Slot } = createSlotFill( 'ToolsPanelSlot' );
const resetAll = jest.fn();

// Default props for the tools panel.
Expand Down Expand Up @@ -310,6 +312,30 @@ describe( 'ToolsPanel', () => {
// Groups should be: default controls, optional controls & reset all.
expect( menuGroups.length ).toEqual( 3 );
} );

it( 'should render placeholder items when panel opts into that feature', () => {
const { container } = render(
<ToolsPanel
{ ...defaultProps }
shouldRenderPlaceholderItems={ true }
>
<ToolsPanelItem { ...altControlProps }>
<div>Optional control</div>
</ToolsPanelItem>
</ToolsPanel>
);

const optionalItem = screen.queryByText( 'Optional control' );
const placeholder = container.querySelector(
'.components-tools-panel-item'
);

// When rendered as a placeholder a ToolsPanelItem will just omit
// all the item's children. So we should still find the container
// element but not the text etc within.
expect( optionalItem ).not.toBeInTheDocument();
expect( placeholder ).toBeInTheDocument();
} );
} );

describe( 'callbacks on menu item selection', () => {
Expand Down Expand Up @@ -425,4 +451,64 @@ describe( 'ToolsPanel', () => {
expect( altMenuItem ).toHaveAttribute( 'aria-checked', 'false' );
} );
} );

describe( 'rendering via SlotFills', () => {
beforeEach( () => {
jest.clearAllMocks();
controlProps.attributes.value = true;
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
} );

it( 'should maintain visual order of controls when toggled on and off', async () => {
// Multiple fills are added to better simulate panel items being
// injected from different locations.
render(
<SlotFillProvider>
<ToolsPanelItems>
<ToolsPanelItem { ...altControlProps }>
<div>Item 1</div>
</ToolsPanelItem>
</ToolsPanelItems>
<ToolsPanelItems>
<ToolsPanelItem { ...controlProps }>
<div>Item 2</div>
</ToolsPanelItem>
</ToolsPanelItems>
<ToolsPanel { ...defaultProps }>
<Slot />
</ToolsPanel>
</SlotFillProvider>
);

// Only the second item should be shown initially as it has a value.
const firstItem = screen.queryByText( 'Item 1' );
const secondItem = screen.getByText( 'Item 2' );

expect( firstItem ).not.toBeInTheDocument();
expect( secondItem ).toBeInTheDocument();

// Toggle on the first item.
await selectMenuItem( altControlProps.label );

// The order of items should be as per their original source order.
let items = screen.getAllByText( /Item [1-2]/ );

expect( items ).toHaveLength( 2 );
expect( items[ 0 ] ).toHaveTextContent( 'Item 1' );
expect( items[ 1 ] ).toHaveTextContent( 'Item 2' );

// Then toggle off both items.
await selectMenuItem( controlProps.label );
await selectMenuItem( altControlProps.label );

// Toggle on controls again and ensure order remains.
await selectMenuItem( controlProps.label );
await selectMenuItem( altControlProps.label );

items = screen.getAllByText( /Item [1-2]/ );

expect( items ).toHaveLength( 2 );
expect( items[ 0 ] ).toHaveTextContent( 'Item 1' );
expect( items[ 1 ] ).toHaveTextContent( 'Item 2' );
} );
} );
} );
13 changes: 9 additions & 4 deletions packages/components/src/tools-panel/tools-panel-item/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,17 @@ const ToolsPanelItem = (
props: WordPressComponentProps< ToolsPanelItemProps, 'div' >,
forwardedRef: Ref< any >
) => {
const { children, isShown, ...toolsPanelItemProps } = useToolsPanelItem(
props
);
const {
children,
isShown,
shouldRenderPlaceholder,
...toolsPanelItemProps
} = useToolsPanelItem( props );

if ( ! isShown ) {
return null;
return shouldRenderPlaceholder ? (
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
<View { ...toolsPanelItemProps } ref={ forwardedRef } />
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should force a aria-hidden="true" on this element to make sure it's ignored by assistive technology — not sure if it matters since it's an empty div 🤷

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes a difference for visual items like pictures or icons.

) : null;
}

return (
Expand Down
17 changes: 12 additions & 5 deletions packages/components/src/tools-panel/tools-panel-item/hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,14 @@ export function useToolsPanelItem(
...otherProps
} = useContextSystem( props, 'ToolsPanelItem' );

const cx = useCx();
const classes = useMemo( () => {
return cx( styles.ToolsPanelItem, className );
}, [ className ] );

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

const hasValueCallback = useCallback( hasValue, [ panelId ] );
Expand Down Expand Up @@ -108,9 +104,20 @@ export function useToolsPanelItem(
? menuItems?.[ menuGroup ]?.[ label ] !== undefined
: isMenuItemChecked;

const placeholderStyle =
shouldRenderPlaceholder &&
! isShown &&
styles.ToolsPanelItemPlaceholder;

const cx = useCx();
const classes = useMemo( () => {
return cx( styles.ToolsPanelItem, placeholderStyle, className );
}, [ isShown, shouldRenderPlaceholder, className ] );

aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
return {
...otherProps,
isShown,
shouldRenderPlaceholder,
className: classes,
};
}
14 changes: 14 additions & 0 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 @@ -67,6 +67,13 @@ export function DimensionPanel( props ) {

## Props

### `hasInnerWrapper`: `boolean`

A function to call when the `Reset all` menu option is selected. This is passed
through to the panel's header component.
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved

- Required: No

### `label`: `string`

Text to be displayed within the panel's header and as the `aria-label` for the
Expand All @@ -88,3 +95,10 @@ A function to call when the `Reset all` menu option is selected. This is passed
through to the panel's header component.

- Required: Yes

### `shouldRenderPlaceholderItems`: `boolean`

Advises the `ToolsPanel` that its child `ToolsPanelItem`s should render
placeholder content instead of null when they are toggled off and hidden.
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved

- Required: No
Loading