Standardize Block Inspector Settings to use ToolsPanel - Tracking Issue

[fabiankaegy]

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:

1. Control over which settings are displayed at any given time
2. Much easier mechanism for injecting additional settings into an existing group
3. 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

[fabiankaegy]

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]

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]

@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]

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]

@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]

@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]

@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]

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]

@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]

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]

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

[jameskoster]

No I don't think so.

[Mayank-Tripathi32]

@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]

@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]

@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]

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.

[hanneslsm]

It's truly amazing to see the incredible speed and collaborative work on this issue!

I've noticed that some panels are positioned left of the inspector while other overlays the inspector. Not sure if this is part of this refactor, but since every panel is touched anyway, we could ensure some consistency here.
I personally would vote for the settings being on the left of the inspector.
Related: #41546

Left	Overlay

[fabiankaegy]

@hanneslsm that is a very good observation! Looking at the code it seems like this here is what is needed for a tools panel to render on the left.

gutenberg/packages/block-library/src/post-featured-image/edit.js

Line 188 in 6a81c61

const dropdownMenuProps = useToolsPanelDropdownMenuProps();

Let's standardize for that as well. I'll add that callout to the PR's that are still open. But the 17 that are already closed should get a follow up for it also 👍 :)

[t-hamano]

Thanks to everyone working on this issue.

As you work through this issue, here's something I'd like you to test:

1. No visual changes: Operate all the controls and make sure there are no changes, especially in spacing (See this comment).
2. Accessibility: Fully operable via keyboard alone, with no loss of focus (See this comment).
3. Blocks do not break: Ideally, we should test this via E2E tests, but they may not cover all scenarios. Try as many different operations as possible and make sure that blocks do not break or that errors are not displayed in the browser console (See this comment).

Especially when it comes to accessibility, issues can sometimes go unnoticed. Not just this issue, but any time you make any changes to the UI, it’s a good idea to do accessibility testing.

[t-hamano]

Additionally, I believe we may need to modify the ToolsPanelItem component itself to fully complete this issue.

Children of the ToolsPanelItem component may not necessarily be form-related. For example, the "Edit menu" button in the Page List block. In such a case, the hasValue() function itself is unnecessary. Furthermore, the reset action would probably not be necessary either.

Therefore, I think at least the following changes are necessary:

• Make the hasValue prop optional
• Allow the user to opt out of the reset action (allowReset, etc.)

cc @WordPress/gutenberg-components

[im3dabasia]

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]

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]

@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]

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 🚀