Tools Panel: only show header ➕ icon when all items are optional #38262
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
…, disables the plus icon. The plus icon should only appear where there are optional menu items only, and only when no optional item is selected. Adding a test to reflect this change.
ramonjd
added
[Type] Enhancement
|
Size Change: +599 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for putting this together so quick @ramonjd 🚀
This tests as advertised for me:
✅ ToolsPanel unit tests pass
✅ New unit tests LGTM
✅ Vertical ellipsis is shown whenever the panel contains default controls or optional control is toggled on
✅ Plus icon is only shown when the panel contains all optional controls and all are hidden
I left a couple of small nitpick comments.
In addition, I think it would be good to update the ToolsPanel storybook example relating to the plus icon use to reflect this PR's changes.
For example, adding a "knob" to the story to toggle on or off inclusion of a default control so the conditions under which the plus icon should be shown can more easily be explored.
Possible update to the ToolsPanel story
diff --git a/packages/components/src/tools-panel/stories/index.js b/packages/components/src/tools-panel/stories/index.js
index f3afeb1190..f36076ee65 100644
--- a/packages/components/src/tools-panel/stories/index.js
+++ b/packages/components/src/tools-panel/stories/index.js
@@ -2,6 +2,7 @@
* External dependencies
*/
import styled from '@emotion/styled';
+import { boolean } from '@storybook/addon-knobs';
/**
* WordPress dependencies
@@ -23,6 +24,9 @@ import { createSlotFill, Provider as SlotFillProvider } from '../../slot-fill';
export default {
title: 'Components (Experimental)/ToolsPanel',
component: ToolsPanel,
+ parameters: {
+ knobs: { disable: false },
+ },
};
export const _default = () => {
@@ -138,19 +142,38 @@ export const WithNonToolsPanelItems = () => {
export const WithOptionalItemsPlusIcon = () => {
const [ height, setHeight ] = useState();
const [ width, setWidth ] = useState();
+ const [ minWidth, setMinWidth ] = useState();
const resetAll = () => {
setHeight( undefined );
setWidth( undefined );
+ setMinWidth( undefined );
};
+ const includeDefaultControls = boolean( 'includeDefaultControls', false );
+
return (
<PanelWrapperView>
<Panel>
<ToolsPanel
label="Tools Panel (optional items only)"
resetAll={ resetAll }
+ key={ includeDefaultControls }
>
+ { includeDefaultControls && (
+ <SingleColumnItem
+ hasValue={ () => !! minWidth }
+ label="Minimum width"
+ onDeselect={ () => setMinWidth( undefined ) }
+ isShownByDefault={ true }
+ >
+ <UnitControl
+ label="Minimum width"
+ value={ minWidth }
+ onChange={ ( next ) => setMinWidth( next ) }
+ />
+ </SingleColumnItem>
+ ) }
<SingleColumnItem
hasValue={ () => !! width }
label="Width"
Also, we'll need an entry in the components package changelog. 🙂
Thanks again for this one!
|
This worked as advertised for me, and I think the UX makes more sense with this change. |
… min-width panel updating isEmpty utility function to not check of object type. TS does that. Updating useEffect dep array.
|
Thanks for the reviews folks! And for the nudge about Storybook knobs @aaronrobertshaw (and for taking the time to make a patch)
|
aaronrobertshaw
added
the
[Feature] UI Components
|
Nice, incredibly fast work! Works as advertised: It's a small change, but for the purposes of clearing values from default controls, this feels like it might help. Additional tweaks we might explore, completely separate from this PR and if necessary, could be a conditional "clear all" icon that shows up next to the ellipsis. |
ciampo
reviewed
Jan 27, 2022
There was a problem hiding this comment.
Thank you @ramonjd for working on this!
@aaronrobertshaw's review already included most of the comments that I would have made 🎉
I just left a couple of minor comments, otherwise the changes in this PR LGTM :)
As a follow-up (with not rush at all), we should refactor this Storybook example as the knobs addon is deprecated. We should look into converting the examples to the new controls approach — here's a couple of references:
| return ( | ||
| <PanelWrapperView> | ||
| <Panel> | ||
| <ToolsPanel | ||
| label="Tools Panel (optional items only)" | ||
| resetAll={ resetAll } | ||
| key={ includeDefaultControls } |
There was a problem hiding this comment.
Is includeDefaultControls used as a key to force ToolsPanel to re-render?
There was a problem hiding this comment.
Yes. Unfortunately without it, the panel doesn't refresh properly. At first I thought it pointed to a bug in the source code, but the storybook and the Gutenberg app are behaving differently here. 🤷
Co-authored-by: Marco Ciampini <[email protected]>
Great to know, thanks @ciampo It's a good opportunity to read up on them. Thanks!! |
ciampo
approved these changes
Jan 27, 2022
ramonjd
deleted the
update/tools-panel-icon-show-plus-for-optional-only
branch
8 tasks
|
Migration to controls over at #38308 |
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.
Description
➕ ➕ ➕ ➕ ➕ ➕ ➕ ➕
This change checks for default ToolsPanel items and, where they exist, disables the plus icon.
The plus icon should only appear where there are optional menu items only, and only when no optional item is selected.
The argument to limit the plus icon in this way is that there is the potential for it to cause confusion as to its purpose, particularly in the presence of a minus sign (where the control has a value).
See discussion over at #38219 (comment) for context
➕ ➕ ➕ ➕ ➕ ➕ ➕ ➕
Testing Instructions
Pick a block that has some form of block supports with default entries for example the List Block
block.jsonhas Typography support.Fire up the editor and check that the ellipses icon for the corresponding panel is always, regardless of which item you select or deselect.
Now, remove the
"__experimentalDefaultControls"object:Check that the plus icon is displayed when there are no optional controls selected.
Blocks that inject controls into the ToolsPanel, e.g., minimum height for the Cover Block, or drop cap for the Paragraph block should work similarly depending on whether the ToolsPanel Item
isShownByDefaultprop is true or false.So if it's
truethe plus icon will not show since there's a default. If it'sfalseit means the fill is optional, so, if there are no other defaults, a plus icon will display.Tests shall pass!
npm run test-unit packages/components/src/tools-panel/test/index.jsEnsure the relevant story is still... relevant.
npm run storybook:devTypes of changes
Design tool icon change.
Checklist:
*.native.jsfiles for terms that need renaming or removal).