Move tools panel to the left of the inspector #55785
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
richtabor
added
[Type] Enhancement
|
The positive.
The negative.
The bottom line is that the left popover is already used today and moving other dropdowns such as Typography to the side brings with it added benefit such as the added space it can take. |
alexstine
added
Needs Accessibility Feedback
|
I'm going to go ahead and say this likely needs accessibility feedback. I am the wrong person to give it though. |
alexstine
removed
the
Needs Dev Note
|
Although the popover as opened from the ellipsis menu is a bit farther from the "opener", the benefits of the dropdown not covering the design tools as they appear in the tools panel makes it a much more usable and ergonomic behavior, so it seems like a solid win. |
|
I agree, this feels like an improvement and solves a couple of existing issues. With the long runway to 6.5 now feels like a good time to merge so we can get a feel for the new behaviour over an extended period of time. |
talldan
reviewed
Nov 2, 2023
packages/components/src/tools-panel/tools-panel-header/component.tsx
Outdated
Show resolved
Hide resolved
I'd agree this is an improvement as not covering the UI the popover items are meant to change is a clear win. Quickly testing, I noticed a couple things that could be improved, although one of them is unrelated to this PR and I'll keep it in a separate comment. First thing:
However, ths visual appearance mismatches the semantics: a default option does look disabled but it does not look selected. Screenshot:
In the above screenshot, there is no visual indication that 'Font size' is selected. I think the selected state should be visually indicated also for disabled default options. |
|
Second thing (unrelated to this PR): When the fly-out tools panel is open, it is still possible to open modal dialogs via the related keyboard shortcuts. In this case, the modal dialog opens and the tools panel stays open appearing on top of the modal dialog overlay. Visually, it just looks not correct. To reproduce:
Screenshots:
It appears to happen with other popovers as well so I'm guessing it's a pre-existing problem. I will create a separate issue. Screenshots of other popovers that stay open on top of the modal dialog overlay:
|
|
Size Change: +113 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Nov 9, 2023
| @@ -199,18 +200,23 @@ const ToolsPanelHeader = ( | |||
| isSmall: true, | |||
| describedBy: dropdownMenuDescriptionText, | |||
| } } | |||
| popoverProps={ dropdownMenuProps } | |||
There was a problem hiding this comment.
If we're passing dropdownMenuProps, then they should be applied to the DropdownMenu component, and not just to its popoverProps prop.
Something like:
Suggested change
| popoverProps={ dropdownMenuProps } | |
| { ...dropdownMenuProps } |
The question then becomes about what prop takes the precedence? Should values from dropdownMenuProps override the props normally passed to DropdownMenu ? Should we merge values in menuProps and toggleProps ? @aaronrobertshaw and @talldan what do you think?
There was a problem hiding this comment.
Gotcha. I wasn't able to get it to function properly using the ...dropdownMenuProps like that, as-is.
There was a problem hiding this comment.
I wasn't able to get it to function properly using the ...dropdownMenuProps like that, as-is.
The missing piece to adopt the { ...dropdownMenuProps } approach is that the props being passed to the component also need a small change. I've included a diff to illustrate in my review.
@aaronrobertshaw and @talldan what do you think?
The experience for the ToolsPanel is fairly curated. That makes me think we don't want the new dropdownMenuProps overriding what the component is doing. Merging might be a compromise option. Do we actually have a use case that requires the extra control yet?
| * | ||
| * @default {} | ||
| */ | ||
| dropdownMenuProps?: {}; |
There was a problem hiding this comment.
The correct type for dropdownMenuProps should be "the props that the DropdownMenu component accepts.
So, something like React.ComponentProps< typeof DropdownMenu > should work. The thing to consider is whether we should Omit any of those props (for example, children).
There was a problem hiding this comment.
Ah, I see. I've updated via ea682b9.
There was a problem hiding this comment.
The thing to consider is whether we should Omit any of those props (for example, children).
What do you think?
There was a problem hiding this comment.
We probably don't want to accept children. It would also make little sense to override props like icon and label, but I wonder if we should just allow that for the sake of extensibility. I'll let @aaronrobertshaw advise on this one, since he's more experienced with ToolsPanel than I am.
Related note: I guess that we drop the dropdownMenuClassName prop from ToolsPanelHeader, and use dropdownMenuProps directly instead?
|
Flaky tests detected in 730a64c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7135379443
|
There was a problem hiding this comment.
Thanks for working on this one @richtabor! I'm glad we've decided on a direction to improve the ToolsPanel dropdown 👍
In general, this is testing pretty well for me. I did notice a couple of small things we still might need to tweak though.
- Should we make the offset for the ToolsPanel dropdowns match the offset for the color popovers? The color popovers look tone offset a couple of pixel more at present.
- The image settings panel in global styles needs the updated popover props as well
- If we are supporting all dropdown menu props, the comments for the types will need to be tweaked as the prop would cover more than just the popover props.
In relation to @ciampo's suggestion, we'd need to tweak the dropdownMenuProps objects passed to the component so they can be spread correctly.
Diff making the above suggestion work and tweaking missed panel
diff --git a/packages/block-editor/src/components/global-styles/border-panel.js b/packages/block-editor/src/components/global-styles/border-panel.js
index ed07bae302..ff1f3ddf55 100644
--- a/packages/block-editor/src/components/global-styles/border-panel.js
+++ b/packages/block-editor/src/components/global-styles/border-panel.js
@@ -16,7 +16,7 @@ import { __ } from '@wordpress/i18n';
*/
import BorderRadiusControl from '../border-radius-control';
import { useColorsPerOrigin } from './hooks';
-import { getValueFromVariable } from './utils';
+import { getValueFromVariable, TOOLSPANEL_DROPDOWNMENU_PROPS } from './utils';
export function useHasBorderPanel( settings ) {
const controls = [
@@ -62,10 +62,7 @@ function BorderToolsPanel( {
label={ __( 'Border' ) }
resetAll={ resetAll }
panelId={ panelId }
- dropdownMenuProps={ {
- placement: 'left-start',
- offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
- } }
+ dropdownMenuProps={ TOOLSPANEL_DROPDOWNMENU_PROPS }
>
{ children }
</ToolsPanel>
diff --git a/packages/block-editor/src/components/global-styles/color-panel.js b/packages/block-editor/src/components/global-styles/color-panel.js
index 7d7a8332af..2d18149502 100644
--- a/packages/block-editor/src/components/global-styles/color-panel.js
+++ b/packages/block-editor/src/components/global-styles/color-panel.js
@@ -27,7 +27,7 @@ import { __, sprintf } from '@wordpress/i18n';
*/
import ColorGradientControl from '../colors-gradients/control';
import { useColorsPerOrigin, useGradientsPerOrigin } from './hooks';
-import { getValueFromVariable } from './utils';
+import { getValueFromVariable, TOOLSPANEL_DROPDOWNMENU_PROPS } from './utils';
import { setImmutably } from '../../utils/object';
export function useHasColorPanel( settings ) {
@@ -129,10 +129,7 @@ function ColorToolsPanel( {
className="color-block-support-panel"
__experimentalFirstVisibleItemClass="first"
__experimentalLastVisibleItemClass="last"
- dropdownMenuProps={ {
- placement: 'left-start',
- offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
- } }
+ dropdownMenuProps={ TOOLSPANEL_DROPDOWNMENU_PROPS }
>
<div className="color-block-support-panel__inner-wrapper">
{ children }
diff --git a/packages/block-editor/src/components/global-styles/dimensions-panel.js b/packages/block-editor/src/components/global-styles/dimensions-panel.js
index f85c05b0af..47b5bd3297 100644
--- a/packages/block-editor/src/components/global-styles/dimensions-panel.js
+++ b/packages/block-editor/src/components/global-styles/dimensions-panel.js
@@ -23,7 +23,7 @@ import { useCallback, Platform } from '@wordpress/element';
/**
* Internal dependencies
*/
-import { getValueFromVariable } from './utils';
+import { getValueFromVariable, TOOLSPANEL_DROPDOWNMENU_PROPS } from './utils';
import SpacingSizesControl from '../spacing-sizes-control';
import HeightControl from '../height-control';
import ChildLayoutControl from '../child-layout-control';
@@ -178,10 +178,7 @@ function DimensionsToolsPanel( {
label={ __( 'Dimensions' ) }
resetAll={ resetAll }
panelId={ panelId }
- dropdownMenuProps={ {
- placement: 'left-start',
- offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
- } }
+ dropdownMenuProps={ TOOLSPANEL_DROPDOWNMENU_PROPS }
>
{ children }
</ToolsPanel>
diff --git a/packages/block-editor/src/components/global-styles/effects-panel.js b/packages/block-editor/src/components/global-styles/effects-panel.js
index 8752156bc2..9a9fd8d125 100644
--- a/packages/block-editor/src/components/global-styles/effects-panel.js
+++ b/packages/block-editor/src/components/global-styles/effects-panel.js
@@ -26,7 +26,7 @@ import { shadow as shadowIcon, Icon, check } from '@wordpress/icons';
/**
* Internal dependencies
*/
-import { getValueFromVariable } from './utils';
+import { getValueFromVariable, TOOLSPANEL_DROPDOWNMENU_PROPS } from './utils';
import { setImmutably } from '../../utils/object';
export function useHasEffectsPanel( settings ) {
@@ -55,10 +55,7 @@ function EffectsToolsPanel( {
label={ __( 'Effects' ) }
resetAll={ resetAll }
panelId={ panelId }
- dropdownMenuProps={ {
- placement: 'left-start',
- offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
- } }
+ dropdownMenuProps={ TOOLSPANEL_DROPDOWNMENU_PROPS }
>
{ children }
</ToolsPanel>
diff --git a/packages/block-editor/src/components/global-styles/filters-panel.js b/packages/block-editor/src/components/global-styles/filters-panel.js
index 4e37a4cc86..7d4e8d9276 100644
--- a/packages/block-editor/src/components/global-styles/filters-panel.js
+++ b/packages/block-editor/src/components/global-styles/filters-panel.js
@@ -28,7 +28,7 @@ import { useCallback, useMemo } from '@wordpress/element';
/**
* Internal dependencies
*/
-import { getValueFromVariable } from './utils';
+import { getValueFromVariable, TOOLSPANEL_DROPDOWNMENU_PROPS } from './utils';
import { setImmutably } from '../../utils/object';
const EMPTY_ARRAY = [];
@@ -82,10 +82,7 @@ function FiltersToolsPanel( {
label={ _x( 'Filters', 'Name for applying graphical effects' ) }
resetAll={ resetAll }
panelId={ panelId }
- dropdownMenuProps={ {
- placement: 'left-start',
- offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
- } }
+ dropdownMenuProps={ TOOLSPANEL_DROPDOWNMENU_PROPS }
>
{ children }
</ToolsPanel>
diff --git a/packages/block-editor/src/components/global-styles/image-settings-panel.js b/packages/block-editor/src/components/global-styles/image-settings-panel.js
index 68054cf129..5ac0aa29b3 100644
--- a/packages/block-editor/src/components/global-styles/image-settings-panel.js
+++ b/packages/block-editor/src/components/global-styles/image-settings-panel.js
@@ -8,6 +8,11 @@ import {
} from '@wordpress/components';
import { __, _x } from '@wordpress/i18n';
+/**
+ * Internal dependencies
+ */
+import { TOOLSPANEL_DROPDOWNMENU_PROPS } from './utils';
+
export function useHasImageSettingsPanel( name, value, inheritedValue ) {
// Note: If lightbox `value` exists, that means it was
// defined via the the Global Styles UI and will NOT
@@ -47,6 +52,7 @@ export default function ImageSettingsPanel( {
label={ _x( 'Settings', 'Image settings' ) }
resetAll={ resetLightbox }
panelId={ panelId }
+ dropdownMenuProps={ TOOLSPANEL_DROPDOWNMENU_PROPS }
>
<ToolsPanelItem
// We use the `userSettings` prop instead of `settings`, because `settings`
diff --git a/packages/block-editor/src/components/global-styles/typography-panel.js b/packages/block-editor/src/components/global-styles/typography-panel.js
index 5c339392a0..d637629e4b 100644
--- a/packages/block-editor/src/components/global-styles/typography-panel.js
+++ b/packages/block-editor/src/components/global-styles/typography-panel.js
@@ -20,7 +20,7 @@ import LetterSpacingControl from '../letter-spacing-control';
import TextTransformControl from '../text-transform-control';
import TextDecorationControl from '../text-decoration-control';
import WritingModeControl from '../writing-mode-control';
-import { getValueFromVariable } from './utils';
+import { getValueFromVariable, TOOLSPANEL_DROPDOWNMENU_PROPS } from './utils';
import { setImmutably } from '../../utils/object';
const MIN_TEXT_COLUMNS = 1;
@@ -146,10 +146,7 @@ function TypographyToolsPanel( {
label={ __( 'Typography' ) }
resetAll={ resetAll }
panelId={ panelId }
- dropdownMenuProps={ {
- placement: 'left-start',
- offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
- } }
+ dropdownMenuProps={ TOOLSPANEL_DROPDOWNMENU_PROPS }
>
{ children }
</ToolsPanel>
diff --git a/packages/block-editor/src/components/global-styles/utils.js b/packages/block-editor/src/components/global-styles/utils.js
index f4adb7a790..9c46cd692f 100644
--- a/packages/block-editor/src/components/global-styles/utils.js
+++ b/packages/block-editor/src/components/global-styles/utils.js
@@ -159,6 +159,13 @@ export const STYLE_PATH_TO_PRESET_BLOCK_ATTRIBUTE = {
'typography.fontFamily': 'fontFamily',
};
+export const TOOLSPANEL_DROPDOWNMENU_PROPS = {
+ popoverProps: {
+ placement: 'left-start',
+ offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
+ },
+};
+
function findInPresetsBy(
features,
blockName,
diff --git a/packages/block-editor/src/components/inspector-controls/block-support-tools-panel.js b/packages/block-editor/src/components/inspector-controls/block-support-tools-panel.js
index 9f44855dc0..fca80671e1 100644
--- a/packages/block-editor/src/components/inspector-controls/block-support-tools-panel.js
+++ b/packages/block-editor/src/components/inspector-controls/block-support-tools-panel.js
@@ -72,8 +72,10 @@ export default function BlockSupportToolsPanel( { children, group, label } ) {
__experimentalFirstVisibleItemClass="first"
__experimentalLastVisibleItemClass="last"
dropdownMenuProps={ {
- placement: 'left-start',
- offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
+ popoverProps: {
+ placement: 'left-start',
+ offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
+ },
} }
>
{ children }
diff --git a/packages/block-library/src/image/image.js b/packages/block-library/src/image/image.js
index c4037d291f..a49087aa3b 100644
--- a/packages/block-library/src/image/image.js
+++ b/packages/block-library/src/image/image.js
@@ -521,8 +521,10 @@ export default function Image( {
label={ __( 'Settings' ) }
resetAll={ resetAll }
dropdownMenuProps={ {
- placement: 'left-start',
- offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
+ popoverProps: {
+ placement: 'left-start',
+ offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
+ },
} }
>
{ ! multiImageSelection && (
diff --git a/packages/block-library/src/query/edit/inspector-controls/index.js b/packages/block-library/src/query/edit/inspector-controls/index.js
index 1ab5a7271b..c8f687c6b5 100644
--- a/packages/block-library/src/query/edit/inspector-controls/index.js
+++ b/packages/block-library/src/query/edit/inspector-controls/index.js
@@ -227,8 +227,10 @@ export default function QueryInspectorControls( props ) {
setQuerySearch( '' );
} }
dropdownMenuProps={ {
- placement: 'left-start',
- offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
+ popoverProps: {
+ placement: 'left-start',
+ offset: 258, // sidebar width (280px) - button width (24px) + border (2px)
+ },
} }
>
{ showTaxControl && (
diff --git a/packages/components/src/tools-panel/tools-panel-header/component.tsx b/packages/components/src/tools-panel/tools-panel-header/component.tsx
index c1ed158937..2de43716b0 100644
--- a/packages/components/src/tools-panel/tools-panel-header/component.tsx
+++ b/packages/components/src/tools-panel/tools-panel-header/component.tsx
@@ -193,6 +193,7 @@ const ToolsPanelHeader = (
</Heading>
{ hasMenuItems && (
<DropdownMenu
+ { ...dropdownMenuProps }
icon={ dropDownMenuIcon }
label={ dropDownMenuLabelText }
menuProps={ { className: dropdownMenuClassName } }
@@ -200,7 +201,6 @@ const ToolsPanelHeader = (
isSmall: true,
describedBy: dropdownMenuDescriptionText,
} }
- popoverProps={ dropdownMenuProps }
>
{ () => (
<>
If you'd like, I can push the changes from the diff above directly to this PR.
Sure thing! Go ahead and take it over. :) |
I wasn't trying to take over your PR but I'm happy to help where possible. I've pushed tweaks for the |
|
Looks like the component tests need updating as well after the MenuGroups were combined. I'll update these as well shortly. |
|
Nice! Agreed on aligning with color tools and the (in progress) shadow controls in terms of placement. |
Can we add the checkmark icon When I open the dropdown I'm always a little confused with the default controls and wonder what this greyed out thing is. By adding the checkmark, it would be clearer it's selected by default. Screenshot before and after:
|
Yes, that's good. I'd like to remove the disabling of default controls anyhow one day. Though I get why we don't enable that—as the controls reset every time you select the block, unless there is a non-default value. |
|
Latest commit adds the |
aaronrobertshaw
force-pushed
the
try/flyout-tools
branch
from
6943fb2
to
730a64c
Compare
|
Looking forward to getting this landed; are there any additional feedback/concerns here? |
I believe all that is left is a final code review. Any further tweaks could be done in follow-ups. |
glendaviesnz
approved these changes
Dec 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
As detailed in #41546 (comment), and part of #41546, I experimented with moving the panel to the side.
One rough edge is how to better position the popover on mobile. Ideas?
Testing Instructions
Screenshots or screencast
flyout.mp4