ToolsPanel: Add subheadings and reset text to tools panel menu #44260
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
aaronrobertshaw
added
[Type] Enhancement
|
@ciampo or @mirka Do you have any suggestions on a better approach to injecting "reset" text into the default control menu items for the At present this PR will add a new An alternative approach to updating `MenuItem`The following patch was a result of initially hacking around exploring options for #44096. It can be applied to trunk and tested if so desired. diff --git a/packages/components/src/tools-panel/stories/index.js b/packages/components/src/tools-panel/stories/index.js
index 803a01eb34..2a550b36cb 100644
--- a/packages/components/src/tools-panel/stories/index.js
+++ b/packages/components/src/tools-panel/stories/index.js
@@ -27,6 +27,7 @@ export default {
export const _default = () => {
const [ height, setHeight ] = useState();
+ const [ maxHeight, setMaxHeight ] = useState();
const [ minHeight, setMinHeight ] = useState();
const [ width, setWidth ] = useState();
@@ -71,7 +72,6 @@ export const _default = () => {
hasValue={ () => !! minHeight }
label="Minimum height"
onDeselect={ () => setMinHeight( undefined ) }
- isShownByDefault={ true }
>
<UnitControl
label="Minimum height"
@@ -79,6 +79,17 @@ export const _default = () => {
onChange={ ( next ) => setMinHeight( next ) }
/>
</ToolsPanelItem>
+ <ToolsPanelItem
+ hasValue={ () => !! maxHeight }
+ label="Maximum height"
+ onDeselect={ () => setMaxHeight( undefined ) }
+ >
+ <UnitControl
+ label="Maximum height"
+ value={ maxHeight }
+ onChange={ ( next ) => setMaxHeight( next ) }
+ />
+ </ToolsPanelItem>
</ToolsPanel>
</Panel>
</PanelWrapperView>
diff --git a/packages/components/src/tools-panel/styles.ts b/packages/components/src/tools-panel/styles.ts
index 7fe2fc0a0a..9c82f740c0 100644
--- a/packages/components/src/tools-panel/styles.ts
+++ b/packages/components/src/tools-panel/styles.ts
@@ -12,7 +12,7 @@ import {
Wrapper as BaseControlWrapper,
} from '../base-control/styles/base-control-styles';
import { LabelWrapper } from '../input-control/styles/input-control-styles';
-import { COLORS, CONFIG } from '../utils';
+import { baseLabelTypography, COLORS, CONFIG } from '../utils';
import { space } from '../ui/utils/space';
const toolsPanelGrid = {
@@ -145,3 +145,32 @@ export const ToolsPanelItemPlaceholder = css`
export const DropdownMenu = css`
min-width: 200px;
`;
+
+// Following provides for rendering "reset" text within a MenuItem.
+export const DefaultControlsItem = css`
+ /**
+ * This high level of specificity is needed to overcome styles for items
+ * with no icon or shortcut.
+ */
+ &&&&& > span {
+ display: flex;
+ /* item text plus icon min-width to match optional items. */
+ min-width: 208px;
+ justify-content: space-between;
+ padding-right: 0;
+ }
+
+ span > span:last-child {
+ flex: 0;
+ color: var( --wp-admin-theme-color-darker-10 );
+ ${ baseLabelTypography }
+ }
+
+ &&[aria-disabled='true'] {
+ opacity: 1;
+
+ span > span:last-child {
+ opacity: 0.3;
+ }
+ }
+`;
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 62a40a966d..414256f5ed 100644
--- a/packages/components/src/tools-panel/tools-panel-header/component.tsx
+++ b/packages/components/src/tools-panel/tools-panel-header/component.tsx
@@ -7,7 +7,7 @@ import type { ForwardedRef } from 'react';
* WordPress dependencies
*/
import { speak } from '@wordpress/a11y';
-import { check, reset, moreVertical, plus } from '@wordpress/icons';
+import { check, moreVertical, plus } from '@wordpress/icons';
import { __, _x, sprintf } from '@wordpress/i18n';
/**
@@ -26,6 +26,7 @@ import type {
} from '../types';
const DefaultControlsGroup = ( {
+ itemClassName,
items,
toggleItem,
}: ToolsPanelControlsGroupProps ) => {
@@ -34,14 +35,14 @@ const DefaultControlsGroup = ( {
}
return (
- <MenuGroup>
+ <MenuGroup label={ __( 'Defaults' ) }>
{ items.map( ( [ label, hasValue ] ) => {
if ( hasValue ) {
return (
<MenuItem
key={ label }
+ className={ itemClassName }
role="menuitem"
- icon={ reset }
label={ sprintf(
// translators: %s: The name of the control being reset e.g. "Padding".
__( 'Reset %s' ),
@@ -59,7 +60,8 @@ const DefaultControlsGroup = ( {
);
} }
>
- { label }
+ <span>{ label }</span>
+ <span>{ __( 'Reset' ) }</span>
</MenuItem>
);
}
@@ -67,12 +69,13 @@ const DefaultControlsGroup = ( {
return (
<MenuItem
key={ label }
+ className={ itemClassName }
role="menuitemcheckbox"
- icon={ check }
isSelected
aria-disabled
>
- { label }
+ <span>{ label }</span>
+ <span>{ __( 'Reset' ) }</span>
</MenuItem>
);
} ) }
@@ -89,7 +92,7 @@ const OptionalControlsGroup = ( {
}
return (
- <MenuGroup>
+ <MenuGroup label={ __( 'Tools' ) }>
{ items.map( ( [ label, isSelected ] ) => {
const itemLabel = isSelected
? sprintf(
@@ -147,6 +150,7 @@ const ToolsPanelHeader = (
) => {
const {
areAllOptionalControlsHidden,
+ defaultControlsItemClassName,
dropdownMenuClassName,
hasMenuItems,
headingClassName,
@@ -197,6 +201,7 @@ const ToolsPanelHeader = (
<DefaultControlsGroup
items={ defaultItems }
toggleItem={ toggleItem }
+ itemClassName={ defaultControlsItemClassName }
/>
<OptionalControlsGroup
items={ optionalItems }
diff --git a/packages/components/src/tools-panel/tools-panel-header/hook.ts b/packages/components/src/tools-panel/tools-panel-header/hook.ts
index 586fd0e7c5..b44dc5bc9f 100644
--- a/packages/components/src/tools-panel/tools-panel-header/hook.ts
+++ b/packages/components/src/tools-panel/tools-panel-header/hook.ts
@@ -33,12 +33,17 @@ export function useToolsPanelHeader(
return cx( styles.ToolsPanelHeading );
}, [ cx ] );
+ const defaultControlsItemClassName = useMemo( () => {
+ return cx( styles.DefaultControlsItem );
+ }, [ cx ] );
+
const { menuItems, hasMenuItems, areAllOptionalControlsHidden } =
useToolsPanelContext();
return {
...otherProps,
areAllOptionalControlsHidden,
+ defaultControlsItemClassName,
dropdownMenuClassName,
hasMenuItems,
headingClassName,
diff --git a/packages/components/src/tools-panel/types.ts b/packages/components/src/tools-panel/types.ts
index 3290b9fe49..b95a50a4aa 100644
--- a/packages/components/src/tools-panel/types.ts
+++ b/packages/components/src/tools-panel/types.ts
@@ -147,6 +147,7 @@ export type ToolsPanelContext = {
};
export type ToolsPanelControlsGroupProps = {
+ itemClassName?: string;
items: [ string, boolean ][];
toggleItem: ( label: string ) => void;
};
|
|
Size Change: +298 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
|
Here's the tools panel today:
Here's after:
I think it's a small but good improvement and I'd be happy to land this one to move things forward. This was the case before as well, and remains the case here: focus is set on the first item in the menu, even if it's disabled. I understand that this is done to prevent a focus loss in the case where you clear the font size which should render it disabled, but it feels sub-optimal. This is something to think about. |
jasmussen
approved these changes
Sep 19, 2022
|
Hmm, to me, this still looks like an enabled menu item and feels clickable (even though there's no hover style): It would be good to get other opinions though. I know in the issue using the same grey color as the Save button was mentioned as a potential option (#44096 (comment)). When using Voiceover and focusing the Font size item, the label is read as 'Font size RESET'. It might be worth making the 'RESET' text aria-hidden, as I think it may end up being confusing. When the button is active it has its own aria-label ('Reset font size'), so it won't be an issue making the 'RESET' text aria-hidden. |
mirka
reviewed
Sep 19, 2022
|
Appreciate the feedback @talldan 🙇
I've tweaked the text color in the disabled state to match the Saved button as suggested.
Good catch! I've added aria-hidden to the reset text as well. It does read a little nicer 👍 Screen.Recording.2022-09-19.at.11.40.22.pm.mp4 |
aaronrobertshaw
force-pushed
the
add/tools-panel-subheadings-and-reset-text
branch
from
9062762
to
ecffad0
Compare
talldan
approved these changes
Sep 20, 2022
There was a problem hiding this comment.
Changes look good to me. Might be good to get a final approval from @mirka, as I'm less familiar with the components package code.
I've tweaked the text color in the disabled state to match the Saved button as suggested. When hovered
How are you feeling about this, @aaronrobertshaw? It looks very subtle to me, and I'm not sure it makes enough of a difference. I don't mind so much if it's changed back to how it was before. Thanks for trying it.
I don't have strong feelings about this one. Personally, I'd probably lean a little towards the current state of things. While the difference is very subtle there's at least some form of additional clue that the menu item is disabled. Perhaps @jasmussen might have some thoughts on the current visuals that might tilt which way we go.
No arguments here 🙂 |
|
Might it be better to only display the 'Reset' button when it's possible to perform the action? I'm not sure how much value there is in seeing the disabled button... it could get quite noisy when there are many reset-able defaults. |
|
If this one can land and still make it to the backports, I'd just press the green button and look at hiding the Reset button separately. |
ciampo
reviewed
Sep 20, 2022
There was a problem hiding this comment.
Chiming in as Lena is AFK today — most feedback seems to be addressed apart from #44260 (comment)
aaronrobertshaw
commented
Sep 21, 2022
There was a problem hiding this comment.
Thank you everyone for the testing, reviews, and feedback 👍
I think this PR is getting close now. The latest changes include:
- Wrapping the reset label's
margin-leftin anrtlcall - Fixing the menu item suffix test
- Omitting the reset label when the default menu item is unchecked i.e. disabled
- Maintaining a consistent menu width after the reset label omission change above
Here's what I'm getting now:
Screen.Recording.2022-09-21.at.6.53.15.pm.mp4
If there is anything else I've missed or needs tweaking before merging. Please let me know 👍
|
Isn't it a little strange to focus disabled buttons in the toolspanel menu?
Could / should the focus skip directly to Height in the screenshot above? |
It is. I think this would be better addressed in a separate PR though so we can land these changes sooner. |
javierarce
added
the
Needs Figma Update
aaronrobertshaw
deleted the
add/tools-panel-subheadings-and-reset-text
branch
22 tasks
This was referenced Mar 10, 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.
Fixes: #44096
What?
ToolsPanelellipsis menuMenuItemto allow more than just an icon or shortcut as a suffixWhy?
Improve the
ToolsPanelellipsis menu UX.How?
MenuItemto allow non icon or shortcut content (i.e. "reset" text)suffixprop for aMenuItemto set reset text for default control items in theToolsPanelToolsPanelstyles to match the mockups in #44096ToolsPanelstories to provide better demonstration of the ellipsis menu for the default storyTesting Instructions
npm run test:unit packages/components/src/menu-itemScreenshots or screencast
Screen.Recording.2022-09-19.at.7.10.31.pm.mp4
Screen.Recording.2022-09-19.at.7.06.35.pm.mp4