Standardize Block Inspector Settings to use ToolsPanel - Tracking Issue #67813

fabiankaegy · 2024-12-11T08:17:53Z

Today, we have two different UI components for grouping controls in the InspectorControls area (Block Sidebar).
The older PanelBody and the newer ToolsPanel.

The ToolsPanel comes with many benefits such as:

Control over which settings are displayed at any given time
Much easier mechanism for injecting additional settings into an existing group
Visual parity with the Styles that already is using the ToolsPanel extensively

There have been one off PR's to handle this conversation from existing PanelBody components to the newer ToolsPanel before:

Cover Block: Refactor setting panel #65432

This issue is meant to be a tracking issue for converting any remaining usages of the PanelBody

The text was updated successfully, but these errors were encountered:

fabiankaegy · 2024-12-11T08:54:20Z

This issue originates out of a discussion I've had with @youknowriad and @Mamaduka so we have some support for it on the engineering side.

I'd love to get some input from @WordPress/gutenberg-design though as this will be a bit of a visual change (some blocks have already made the move)

youknowriad · 2024-12-11T09:30:37Z

What's not clear to me yet is whether we should go directly towards InspectorControl group="something" or if we do it in steps, first move to ToolsPanel

fabiankaegy · 2024-12-11T09:34:48Z

@youknowriad I think the two are unrelated. This ticket is only concerned with moving to the ToolsPanel UI. #67814 is for coming up with how to allow the extension of those via groups.

That being said, looking at the list above 90% of the instances here simply are a PanelGroup called "Settings" so we should already keep track of what groups there are and potentially consolidate where possible

hanneslsm · 2024-12-11T12:40:24Z

The UX in #65432 (review) is different than the current UX, e.g. in the typography settings. Currently it's about displaying the settings, the new one is about the values.

I think it is crucial to update the display settings simultaneously to ensure consistency and prevent major UX issues.

CleanShot.2024-12-11.at.13.31.58.mp4

fabiankaegy · 2024-12-11T12:58:05Z

@hanneslsm can you elaborate a but? I'm not sure I understand what you mean? What you show for the typography settings here is exactly what was applied in #65432

hanneslsm · 2024-12-11T13:30:01Z

@fabiankaegy Sorry for not being clear enough. I can only speak about the design and was referring to:

I'd love to get some input from @WordPress/gutenberg-design though as this will be a bit of a visual change (some blocks have already made the move)

I was trying to say that the panel (no matter how it's implemented) uses two different UX principles. One (e.g. typography settings) is about resetting what is displayed and the other (e.g. #65432 (review)) is about resetting the values. So, it's not only a visual change but a UX change.

I wanted to point out that we could run into frustrating UX because two panels, which look similar, are controlling two different things. Maybe this is a more a design issue than going from PanelBody to ToolsPanel - I now realize that this issue is probably be not the right place.

fabiankaegy · 2024-12-11T13:39:24Z

@fabiankaegy Sorry for not being clear enough. I can only speak about the design and was referring to:

I'd love to get some input from @WordPress/gutenberg-design though as this will be a bit of a visual change (some blocks have already made the move)

I was trying to say that the panel (no matter how it's implemented) uses two different UX principles. One (e.g. typography settings) is about resetting what is displayed and the other (e.g. #65432 (review)) is about resetting the values. So, it's not only a visual change but a UX change.

I wanted to point out that we could run into frustrating UX because two panels, which look similar, are controlling two different things. Maybe this is a more a design issue than going from PanelBody to ToolsPanel - I now realize that this issue is probably be not the right place.

Ahh okay yeah I see how the two implementations of the ToolsPanel look different here. I wonder why that is though... But yeah ideally it should all be the same

jameskoster · 2024-12-11T16:47:59Z

I'm all for consistency :)

@hanneslsm I don't think the UX is different, it's just that in the Cover block all the setting controls are visible regardless, whereas in the Typography panel only 'Size' is forced, and everything else is optional.

It's not entirely clear why this decision was made, but it begs the question... do we apply this approach to all Settings panels, or are there some where only certain settings would be added by default and others optional?

If it's the former, then this UI doesn't feel particularly intuitive:

The checkmarks are kind of irrelevant since you cannot toggle any settings off.

fabiankaegy · 2024-12-11T17:05:49Z

@jameskoster My idea right now would be to start having all elements that currently are always visible (so all items =D) also be visible after the refactor. That way there are no "regressions" just from us moving component.

Then as a next step we can actually do an audit and choose which controls we want to demote and no longer show by default.

Which is then paired with the filterable defaultControls work we are doing in #67729

jameskoster · 2024-12-11T17:13:00Z

Yes that makes sense, though will lead to a multitude of confusing menu instances as outlined in my previous comment. Maybe it's time to refresh the design a little (separately of course). Something more like the property visibility toggle UX in data views might work—there's a lot of conceptual overlap.

fabiankaegy · 2024-12-11T17:14:47Z

@jameskoster Do you think we should hold on the refactor till we have improved the tools panel? 🤔

jameskoster · 2024-12-11T18:13:03Z

No I don't think so.

Mayank-Tripathi32 · 2024-12-12T17:01:11Z

@fabiankaegy I was reviewing the components for the issues but had a question:

Should we update the implementation for native as well in the same PR? (e.g., edit.native.js)

fabiankaegy · 2024-12-12T17:06:40Z

@Mayank-Tripathi32 That is a good question 👀 I don't know the answer to that. I've never worked with any of the mobile code base

Mayank-Tripathi32 · 2024-12-12T17:20:50Z

@Mayank-Tripathi32 That is a good question 👀 I don't know the answer to that. I've never worked with any of the mobile code base

Interesting. I'll open a PR for edit.js for now. since I can't test the native files.

Mayank-Tripathi32 · 2024-12-12T19:57:16Z

I am working on following components:

[x] - Refactor "Settings" panel of Gallery block to use ToolsPanel instead of PanelBody. (Test cases Pending)
[x] - Refactor "Settings" panel of Details block to use ToolsPanel instead of PanelBody.
[x] - Refactor "Settings" panel of More block to use ToolsPanel instead of PanelBody.
[] - Refactor "Settings" panel of Social Icons block to use ToolsPanel instead of PanelBody
[] - Refactor "Settings" panel of Social Icon block to use ToolsPanel instead of PanelBody

For visibility. Will be opening PR once ready.

im3dabasia · 2024-12-16T11:08:46Z

As per discussion with @fabiankaegy link , I have opened a draft pull request to add dropdown menu properties for all instances that previously did not have dropdown menu props.

For visibility. Will be opening PR once ready. Draft PR: #68019

cc : @hanneslsm

aaronrobertshaw · 2024-12-18T08:59:18Z

Children of the ToolsPanelItem component may not necessarily be form-related.

@t-hamano the ToolsPanelItem component was intended as a means to allow items to register themselves for the ToolsPanel menu which itself facilitated controls to be toggled on and off and reset. The hasValue prop was required to display the menu item correctly i.e. to indicate if it had a value or display that could be reset.

I've been out of the loop on these efforts to use the ToolsPanel everywhere and don't have any real context, so take this with a grain of salt.

Why are we rendering items in ToolsPanelItem components that don't need to be in the menu and can't have their visibility toggled or value reset?

Anything can be rendered as a child to the ToolsPanel, they don't have to be wrapped in a ToolsPanelItem. The color contrast checkers are a current example of this but it could be anything like additional help text, a notice something has moved etc. Even an Edit Menu button for the Page List block settings that is always displayed and serves no purpose in the menu 🙂

Without full context, I'm not saying there can't be tweaks to the ToolsPanel component props but it might help to start from a place where the ToolsPanelItem isn't sort of being "misused" for lack of a better term.

Maybe some best practice docs for the ToolsPanel components would help too?

Here's a quick diff off trunk showing that the PageList edit menu button could have skipped the `ToolsPanelItem`

diff --git a/packages/block-library/src/page-list/edit.js b/packages/block-library/src/page-list/edit.js
index ef927eccecc..8f1409f864f 100644
--- a/packages/block-library/src/page-list/edit.js
+++ b/packages/block-library/src/page-list/edit.js
@@ -359,24 +359,18 @@ export default function PageListEdit( {
 					) }
 
 					{ allowConvertToLinks && (
-						<ToolsPanelItem
-							label={ __( 'Edit Menu' ) }
-							isShownByDefault
-							hasValue={ () => false }
-						>
-							<div>
-								<p>{ convertDescription }</p>
-								<Button
-									__next40pxDefaultSize
-									variant="primary"
-									accessibleWhenDisabled
-									disabled={ ! hasResolvedPages }
-									onClick={ convertToNavigationLinks }
-								>
-									{ __( 'Edit' ) }
-								</Button>
-							</div>
-						</ToolsPanelItem>
+						<div style={ { gridColumn: '1 / -1' } }>
+							<p>{ convertDescription }</p>
+							<Button
+								__next40pxDefaultSize
+								variant="primary"
+								accessibleWhenDisabled
+								disabled={ ! hasResolvedPages }
+								onClick={ convertToNavigationLinks }
+							>
+								{ __( 'Edit' ) }
+							</Button>
+						</div>
 					) }
 				</ToolsPanel>
 			</InspectorControls>

The result of the above diff would give us:

t-hamano · 2024-12-18T09:39:27Z

@aaronrobertshaw Thanks for the feedback!

Why are we rendering items in ToolsPanelItem components that don't need to be in the menu and can't have their visibility toggled or value reset?

Anything can be rendered as a child to the ToolsPanel, they don't have to be wrapped in a ToolsPanelItem.

I mistakenly thought that anything inside a ToolsPanel component had to be wrapped in a ToolsPanelItem 😅

I would like to submit a follow-up PR regarding #68076.

aaronrobertshaw · 2024-12-18T09:50:43Z

I mistakenly thought that anything inside a ToolsPanel component had to be wrapped in a ToolsPanelItem 😅

Easy done. In case it helps here's a link to the Storybook examples for the ToolsPanel illustrating that non-ToolsPanelItems could be included.

I should have tracked that down for my last comment.

I would like to submit a follow-up PR regarding #68076.

You're awesome, thanks Aki 🚀

t-hamano · 2025-01-13T06:58:01Z

While reviewing submitted PRs, I noticed that the order of the controls and the order of the dropdown items did not match.

One example is #67952. This block control conditionally renders "Open in new tab". This can result in that item being placed at the end of the dropdown:

We'll need to check whether this behavior is acceptable for now, or whether we need to find a solution.

Mamaduka · 2025-01-13T07:01:27Z

Additionally, let's update the issue description and include best practices and recommendations for refactoring. This should help fast-track reviews.

aaronrobertshaw · 2025-01-13T07:16:49Z

We'll need to check whether this behavior is acceptable for now, or whether we need to find a solution.

@t-hamano this issue also comes into play when slots can't guarantee their fills order. There's already a solution in place to handle that which revolves around placeholder DOM elements. See the block support tools panels here.

t-hamano · 2025-01-14T10:50:46Z

There's already a solution in place to handle that which revolves around placeholder DOM elements. See the block support tools panels here.

I tested #67952 (Avatar block) with the following changes:

diff --git a/packages/block-library/src/avatar/edit.js b/packages/block-library/src/avatar/edit.js
index 19c81884e8..ddfaa9d28a 100644
--- a/packages/block-library/src/avatar/edit.js
+++ b/packages/block-library/src/avatar/edit.js
@@ -47,6 +47,7 @@ const AvatarInspectorControls = ( {
                                                userId: undefined,
                                        } );
                                } }
+                               shouldRenderPlaceholderItems
                                dropdownMenuProps={ dropdownMenuProps }
                        >
                                <ToolsPanelItem

Unfortunately, this code doesn't seem to work as expected.

My guess is that this prop may not apply to items inside a dropdown.

For example, the Avatar block expects the controls to be ordered as follows:

Image Size
Link to user profile
Open in new tab
User

However, the order of the drop-down items may be as follows:

Image Size
Link to user profile
User
Open in new tab

7d301eec6850d21c2164f2c374aa9f1c.mp4

aaronrobertshaw · 2025-01-15T00:14:20Z

My guess is that this prop may not apply to items inside a dropdown.

Without looking at the code again, I'd say that's right. Check out the ToolsPanelItem or ToolsPanel components to see how shouldRenderPlaceholderItems helps render a placeholder DOM element to maintain the order of fills. That should help narrow in on what could be created to assist panel's rendering non-ToolsPanelItem controls.

t-hamano · 2025-01-19T15:53:44Z

While reviewing submitted PRs, I noticed that the order of the controls and the order of the dropdown items did not match.
One example is #67952. This block control conditionally renders "Open in new tab". This can result in that item being placed at the end of the dropdown:

I studied the implementation and logic of the ToolsPanel component for a while to solve this problem.

In the block sidebar, ToolsPanelItems are rendered conditionally in the ToolsPanel as follows:

<ToolsPanel>
	<ToolsPanelItem label={ __( 'Display as dropdown' ) } />
	{ displayAsDropdown && <ToolsPanelItem label={ __( 'Show label' ) } /> }
	<ToolsPanelItem label={ __( 'Show post counts' ) } />
	<ToolsPanelItem label={ __( 'Group by' ) } />
</ToolsPanel>

I thought about how to make the order of menu items match the actual rendered controls, but with my knowledge, I could only think of the following approach. This is probably not the correct way:

In the ToolsPanel component, loop through the children and get the labels of the ToolsPanelItems. This is the basis for the order of the menu items.
The array that stores the order of the labels is passed to the ToolsPanelHeader component as context.
Sort the items in the default and option groups based on this array.

Example

const UnconnectedToolsPanel = () => {
	// Parse the children and collect the label of the ToolsPanelItem component that will be rendered.
	const menuItemOrder: string[] = [];
	Children.forEach( children, ( child ) => {
		if ( isValidElement( child ) && child.type === ToolsPanelItem && child.props.label
		) {
			menuItemOrder.push( child.props.label );
		}
	} );

	return (
		<Grid { ...toolsPanelProps } columns={ 2 } ref={ forwardedRef }>
			<ToolsPanelContext.Provider
				// Injects the label ordering into the context.
				value={ { ...panelContext, menuItemOrder } }
			>
				<ToolsPanelHeader />
				{ children }
			</ToolsPanelContext.Provider>
		</Grid>
	);
};

The logic of the ToolsPanel component is very complicated, and I may not fully understand it. If you have any other good ideas, please let me know.

aaronrobertshaw · 2025-01-20T02:33:25Z

Thanks for digging into this @t-hamano, as you note, the component's logic is pretty complicated.

In the ToolsPanel component, loop through the children and get the labels of the ToolsPanelItems. This is the basis for the order of the menu items.

I'm not sure about this one.

We still need the ToolsPanel to handle the slot fill use case and there we can't guarantee the order of fills and therefore the children. So while we might achieve the menu matching the control order in this edge case of an optional conditional control being randomly rendered in the middle of other controls, it might come at the price of the overall order of both controls and menu items changing at times.

Unfortunately, I'm short on time to be able to dig in and confirm all this.

With the recent discussion around stabilizing the ToolsPanel components and what API improvements might be required to achieve that, I think this use case could be a part of that. Perhaps we could extend the ToolsPanelItem API to flag a related control that it must follow, or declare a ToolsPanelItemGroup to wrap related controls or something?

For the time being, I'd recommend against hacking around the ToolsPanel menu item order context and state.

I appreciate the Open in new tab menu item appearing at the bottom due to it being conditionally rendered isn't ideal. It looks like this edge case has also crept in with the Media Text block as seen below.

I don't think this needs to be a blocker though if people think the switch to ToolsPanels across the board is more important than this UI quirk/bug. I see it as a side effect of the rush to adopt a component that was still officially __experimental.

t-hamano · 2025-01-20T04:34:37Z

I don't think this needs to be a blocker though if people think the switch to ToolsPanels across the board is more important than this UI quirk/bug.

To help us decide on this, I've put together a list of blocks where the order of controls and the order of menu items may differ.

I would like to apply the ToolsPanel consistently across all blocks and I think the order of menu items is acceptable for now, but what do you think?

Blocks where the ToolsPanel component has been applied

Media & Text

Terms List

The PR has not yet been merged, but there are some blocks that may cause problems when applying the ToolsPanel component

fabiankaegy added [Package] Block library /packages/block-library [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. labels Dec 11, 2024

fabiankaegy mentioned this issue Dec 11, 2024

Standardize the usage of the InspectorControls "group" slots for all ToolsPanel components we provide in Gutenberg #67814

Open

im3dabasia mentioned this issue Dec 12, 2024

Archives Block: Refactor setting panel #67841

Merged

Mayank-Tripathi32 mentioned this issue Dec 12, 2024

Button Block: Refactor setting panel #67887

Merged

This was referenced Dec 12, 2024

Refactor "Settings" panel of Archives block to use ToolsPanel instead of PanelBody #67890

Closed

Refactor "Settings" panel of Button block to use ToolsPanel instead of PanelBody #67892

Closed

This was referenced Dec 12, 2024

Refactor "Settings" panel of Gallery block to use ToolsPanel instead of PanelBody #67893

Open

Refactor "Settings" panel of Details block to use ToolsPanel instead of PanelBody #67894

Closed

This was referenced Dec 12, 2024

Table Block: Refactor settings panel to use ToolsPanel #67896

Merged

Site Title Block: Refactor settings panel to use ToolsPanel #67898

Merged

This was referenced Dec 12, 2024

Refactor "Settings" panel of Table block to use ToolsPanel instead of PanelBody #67900

Closed

Refactor "Settings" panel of Site Title block to use ToolsPanel instead of PanelBody #67901

Closed

im3dabasia mentioned this issue Dec 13, 2024

Page List Block: Refactor settings panel to use ToolsPanel #67903

Merged

himanshupathak95 mentioned this issue Dec 16, 2024

Query: Refactor settings panel to use ToolsPanel #68008

Merged

Infinite-Null mentioned this issue Dec 16, 2024

Login/Logout: Add dropdown menu props to ToolsPanel component #68009

Merged

im3dabasia mentioned this issue Dec 16, 2024

Archive: Add dropdown menu props to ToolsPanel component #68010

Merged

SainathPoojary mentioned this issue Dec 16, 2024

Navigation: Refactor display panel to use ToolsPanel #68011

Open

im3dabasia mentioned this issue Dec 16, 2024

Page List Block: Add dropdown menu props to ToolsPanel component #68012

Merged

This was referenced Dec 16, 2024

Table Block: Add dropdown menu props to ToolsPanel component #68016

Closed

Site Title Block: Add dropdown menu props to ToolsPanel component #68017

Merged

Date Block: Add dropdown menu props to ToolsPanel component #68018

Merged

im3dabasia mentioned this issue Dec 16, 2024

Add dropdown menu props to ToolsPanel component #68019

Merged

fabiankaegy mentioned this issue Dec 17, 2024

ToolsPanel: Selected ToolsPanel menu options are not persistent #41544

Open

Infinite-Null mentioned this issue Dec 17, 2024

Add Tools Panel dropdown menu props to More block #68039

Merged

t-hamano mentioned this issue Dec 18, 2024

Page List Block: Fix critical error when converting to link #68076

Merged

up1512001 mentioned this issue Dec 19, 2024

Video Block: refactor setting panel #67044

Merged

jameskoster mentioned this issue Dec 20, 2024

Audit and improve ToolsPanel's a11y #38059

Open

This was referenced Dec 23, 2024

Page List block: Don't wrap Edit button with ToolsPanelItem component #68248

Merged

Refactor "Settings" panel of Video block to use ToolsPanel instead of PanelBody #68341

Closed

Sukhendu2002 mentioned this issue Jan 8, 2025

Title Block: Refactor settings panel to use ToolsPanel #68544

Closed

Standardize Block Inspector Settings to use ToolsPanel - Tracking Issue #67813

Standardize Block Inspector Settings to use ToolsPanel - Tracking Issue #67813

Comments

fabiankaegy commented Dec 11, 2024 • edited Loading

fabiankaegy commented Dec 11, 2024

youknowriad commented Dec 11, 2024

fabiankaegy commented Dec 11, 2024

hanneslsm commented Dec 11, 2024 • edited Loading

fabiankaegy commented Dec 11, 2024

hanneslsm commented Dec 11, 2024

fabiankaegy commented Dec 11, 2024

jameskoster commented Dec 11, 2024

fabiankaegy commented Dec 11, 2024

jameskoster commented Dec 11, 2024

fabiankaegy commented Dec 11, 2024

jameskoster commented Dec 11, 2024

Mayank-Tripathi32 commented Dec 12, 2024

fabiankaegy commented Dec 12, 2024

Mayank-Tripathi32 commented Dec 12, 2024

Mayank-Tripathi32 commented Dec 12, 2024 • edited Loading

im3dabasia commented Dec 16, 2024 • edited Loading

aaronrobertshaw commented Dec 18, 2024

t-hamano commented Dec 18, 2024

aaronrobertshaw commented Dec 18, 2024 • edited Loading

t-hamano commented Jan 13, 2025 • edited Loading

Mamaduka commented Jan 13, 2025

aaronrobertshaw commented Jan 13, 2025

t-hamano commented Jan 14, 2025

aaronrobertshaw commented Jan 15, 2025

t-hamano commented Jan 19, 2025

aaronrobertshaw commented Jan 20, 2025

t-hamano commented Jan 20, 2025

Archives

Media & Text

Terms List

Navigation

Avatar

Site Logo

Author

Post Navigation Link

fabiankaegy commented Dec 11, 2024 •

edited

Loading

hanneslsm commented Dec 11, 2024 •

edited

Loading

Mayank-Tripathi32 commented Dec 12, 2024 •

edited

Loading

im3dabasia commented Dec 16, 2024 •

edited

Loading

aaronrobertshaw commented Dec 18, 2024 •

edited

Loading

t-hamano commented Jan 13, 2025 •

edited

Loading