Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ToolsPanel: switch to plus icon when no controls present in panel body #34107

Merged
merged 12 commits into from
Oct 14, 2021

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Aug 17, 2021

Depends on: #34530

Today we're swapping the ellipsis icon with a plus symbol when there are no selected controls within a ToolsPanel.

Screen Shot 2021-08-17 at 8 03 46 pm

The original suggestion. Thank you!

The motivation is to provide a hint to users that they can add optional controls to the panel.

"Optional" controls – introduced in #34530 – are those not displayed by default, and whose visibility may be toggled.

Some known edge cases/considerations:

  • When the ToolsPanel has children that are not rendered (and therefore selectable) in the panel menu. Not sure yet, but we might have to know whether there are children that not registered ToolsPanelItems.

Several ToolsPanels will therefore look something like this:

Screen Shot 2021-08-17 at 8 18 38 pm

Testing

  1. Fire up this branch, and insert a group, cover or other block that has a dimensions ToolsPanel in the sidebar controls.
  2. If there is no optional control present in the panel body, the plus icon should display in the ToolsPanel header. "Default" controls cannot be toggled and should have no effect on the icon.
  3. Toggle controls on and off in the ToolsPanel menu. The ellipsis should show when there are controls present in the panel body.
  4. Check that panel with only default controls, and no optional controls, always show the vertical dots icon.
  5. Check the aria-label for both button states. They should reflect the label prop passed down to the dropdown component.

Run npm run test-unit packages/components/src/tools-panel/test/index.js for glory!

Screenshots

double-plus.mp4

New view in storybook:

Screen Shot 2021-09-15 at 10 40 03 pm

What this PR does

Types of changes

UI: Adding an icon to the ToolsPanel header.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@ramonjd ramonjd added [Type] Enhancement A suggestion for improvement. [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Aug 17, 2021
@ramonjd ramonjd self-assigned this Aug 17, 2021
@ramonjd ramonjd added the [Status] In Progress Tracking issues with work in progress label Aug 17, 2021
@github-actions
Copy link

github-actions bot commented Aug 17, 2021

Size Change: +93 B (0%)

Total Size: 1.07 MB

Filename Size Change
build/components/index.min.js 217 kB +93 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 931 B
build/admin-manifest/index.min.js 1.09 kB
build/annotations/index.min.js 2.7 kB
build/api-fetch/index.min.js 2.21 kB
build/autop/index.min.js 2.08 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.2 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-editor/index.min.js 134 kB
build/block-editor/style-rtl.css 13.9 kB
build/block-editor/style.css 13.9 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 474 B
build/block-library/blocks/button/editor.css 474 B
build/block-library/blocks/button/style-rtl.css 600 B
build/block-library/blocks/button/style.css 600 B
build/block-library/blocks/buttons/editor-rtl.css 315 B
build/block-library/blocks/buttons/editor.css 315 B
build/block-library/blocks/buttons/style-rtl.css 370 B
build/block-library/blocks/buttons/style.css 370 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 131 B
build/block-library/blocks/code/theme.css 131 B
build/block-library/blocks/columns/editor-rtl.css 206 B
build/block-library/blocks/columns/editor.css 205 B
build/block-library/blocks/columns/style-rtl.css 497 B
build/block-library/blocks/columns/style.css 496 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/cover/style-rtl.css 1.17 kB
build/block-library/blocks/cover/style.css 1.17 kB
build/block-library/blocks/embed/editor-rtl.css 488 B
build/block-library/blocks/embed/editor.css 488 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 977 B
build/block-library/blocks/gallery/editor.css 982 B
build/block-library/blocks/gallery/style-rtl.css 1.6 kB
build/block-library/blocks/gallery/style.css 1.59 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 114 B
build/block-library/blocks/heading/style.css 114 B
build/block-library/blocks/home-link/style-rtl.css 247 B
build/block-library/blocks/home-link/style.css 247 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 731 B
build/block-library/blocks/image/editor.css 730 B
build/block-library/blocks/image/style-rtl.css 502 B
build/block-library/blocks/image/style.css 505 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 493 B
build/block-library/blocks/media-text/style.css 490 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 568 B
build/block-library/blocks/navigation-link/editor.css 570 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 300 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/style-rtl.css 195 B
build/block-library/blocks/navigation-submenu/style.css 195 B
build/block-library/blocks/navigation-submenu/view.min.js 343 B
build/block-library/blocks/navigation/editor-rtl.css 1.72 kB
build/block-library/blocks/navigation/editor.css 1.72 kB
build/block-library/blocks/navigation/style-rtl.css 1.62 kB
build/block-library/blocks/navigation/style.css 1.61 kB
build/block-library/blocks/navigation/view.min.js 2.74 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 377 B
build/block-library/blocks/page-list/editor.css 377 B
build/block-library/blocks/page-list/style-rtl.css 198 B
build/block-library/blocks/page-list/style.css 198 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 273 B
build/block-library/blocks/paragraph/style.css 273 B
build/block-library/blocks/post-author/editor-rtl.css 210 B
build/block-library/blocks/post-author/editor.css 210 B
build/block-library/blocks/post-author/style-rtl.css 182 B
build/block-library/blocks/post-author/style.css 181 B
build/block-library/blocks/post-comments-form/style-rtl.css 140 B
build/block-library/blocks/post-comments-form/style.css 140 B
build/block-library/blocks/post-comments/style-rtl.css 360 B
build/block-library/blocks/post-comments/style.css 359 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 396 B
build/block-library/blocks/post-featured-image/editor.css 397 B
build/block-library/blocks/post-featured-image/style-rtl.css 156 B
build/block-library/blocks/post-featured-image/style.css 156 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 391 B
build/block-library/blocks/post-template/style.css 392 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 60 B
build/block-library/blocks/post-title/style.css 60 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 378 B
build/block-library/blocks/pullquote/style.css 378 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 262 B
build/block-library/blocks/query-pagination/editor.css 255 B
build/block-library/blocks/query-pagination/style-rtl.css 234 B
build/block-library/blocks/query-pagination/style.css 231 B
build/block-library/blocks/query-title/editor-rtl.css 85 B
build/block-library/blocks/query-title/editor.css 85 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 187 B
build/block-library/blocks/quote/style.css 187 B
build/block-library/blocks/quote/theme-rtl.css 220 B
build/block-library/blocks/quote/theme.css 222 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 374 B
build/block-library/blocks/search/style.css 375 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 250 B
build/block-library/blocks/separator/style.css 250 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 769 B
build/block-library/blocks/site-logo/editor.css 769 B
build/block-library/blocks/site-logo/style-rtl.css 165 B
build/block-library/blocks/site-logo/style.css 165 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 165 B
build/block-library/blocks/social-link/editor.css 165 B
build/block-library/blocks/social-links/editor-rtl.css 812 B
build/block-library/blocks/social-links/editor.css 811 B
build/block-library/blocks/social-links/style-rtl.css 1.3 kB
build/block-library/blocks/social-links/style.css 1.3 kB
build/block-library/blocks/spacer/editor-rtl.css 307 B
build/block-library/blocks/spacer/editor.css 307 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 146 B
build/block-library/blocks/tag-cloud/style.css 146 B
build/block-library/blocks/template-part/editor-rtl.css 636 B
build/block-library/blocks/template-part/editor.css 635 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/term-description/editor-rtl.css 90 B
build/block-library/blocks/term-description/editor.css 90 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 571 B
build/block-library/blocks/video/editor.css 572 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 853 B
build/block-library/common.css 849 B
build/block-library/editor-rtl.css 9.83 kB
build/block-library/editor.css 9.83 kB
build/block-library/index.min.js 148 kB
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/style-rtl.css 10.4 kB
build/block-library/style.css 10.4 kB
build/block-library/theme-rtl.css 665 B
build/block-library/theme.css 669 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/blocks/index.min.js 45.7 kB
build/components/style-rtl.css 15.2 kB
build/components/style.css 15.2 kB
build/compose/index.min.js 10.4 kB
build/core-data/index.min.js 12.4 kB
build/customize-widgets/index.min.js 11.2 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 614 B
build/data/index.min.js 7.1 kB
build/date/index.min.js 31.5 kB
build/deprecated/index.min.js 428 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.46 kB
build/edit-navigation/index.min.js 15.3 kB
build/edit-navigation/style-rtl.css 3.74 kB
build/edit-navigation/style.css 3.74 kB
build/edit-post/classic-rtl.css 492 B
build/edit-post/classic.css 494 B
build/edit-post/index.min.js 29.3 kB
build/edit-post/style-rtl.css 7.2 kB
build/edit-post/style.css 7.19 kB
build/edit-site/index.min.js 29.7 kB
build/edit-site/style-rtl.css 5.53 kB
build/edit-site/style.css 5.53 kB
build/edit-widgets/index.min.js 15.7 kB
build/edit-widgets/style-rtl.css 4.1 kB
build/edit-widgets/style.css 4.1 kB
build/editor/index.min.js 37.5 kB
build/editor/style-rtl.css 3.78 kB
build/editor/style.css 3.77 kB
build/element/index.min.js 3.17 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 5.93 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.55 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.6 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.72 kB
build/keycodes/index.min.js 1.3 kB
build/list-reusable-blocks/index.min.js 1.85 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 845 B
build/nux/index.min.js 2.03 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.83 kB
build/primitives/index.min.js 921 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 B
build/redux-routine/index.min.js 2.63 kB
build/reusable-blocks/index.min.js 2.19 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.6 kB
build/server-side-render/index.min.js 1.52 kB
build/shortcode/index.min.js 1.48 kB
build/token-list/index.min.js 562 B
build/url/index.min.js 1.74 kB
build/viewport/index.min.js 1.02 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 7.11 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a small change this one, but such a nice improvement @ramonjd! Looks great:

tools-panel-plus-icon-sml

Looks like the linting CI step is failing because of prettier complaining about line length, but other than that, LGTM!

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter looks happy now, and it's still working well. Thanks for updating, LGTM! 🚀

@ramonjd
Copy link
Member Author

ramonjd commented Aug 18, 2021

Looks like the linting CI step is failing because of prettier complaining about line length, but other than that, LGTM!

Thanks for testing @andrewserong My local linter has stopped pulling me up on this for some reason pre-commit 💀

When the ToolsPanel has children that are not rendered (and therefore selectable) in the panel menu. Not sure yet, but we might have to know whether there are children that not registered ToolsPanelItems.

My only idea so far on how to get around this was to check the immediate children of <ToolsPanel /> for components that are not of type ToolsPanelItem (and therefore not registered to appear in the header toolbar).

So, in the tools-panel/hook, something like:

	// Checks for the existence of children that are not ToolPanelItems.
	const [ hasOtherChildren, setHasOtherChildren ] = useState( false );

	useEffect( () => {
		const otherChildren = Children.toArray( props.children ).filter(
			( { type } ) => type?.displayName !== 'ToolsPanelItem'
		);

		setHasOtherChildren( otherChildren.length > 0 );
	}, [ props.children ] );

which we'd add to context and access in the tools header to make our decision.

Needs some more testing, so not sure whether it should be part of this PR or not.

@andrewserong
Copy link
Contributor

Needs some more testing, so not sure whether it should be part of this PR or not.

Good question! The fallback behaviour of displaying the three dots if there are non panel item children might not be too bad... unless it gets tripped up once the SlotFill is added, in which case it might be that the plus icon never gets shown (if the Slot gets counted) 🤔

I think because the fallback behaviour is to show the three dots (like we do currently), I'd personally lean toward going with this and tweaking in follow-ups as we encounter issues. But happy to go either way — apologies for the early/eager review if you want to take a bit longer to explore your options! 😀

@ramonjd
Copy link
Member Author

ramonjd commented Aug 18, 2021

But happy to go either way — apologies for the early/eager review if you want to take a bit longer to explore your options!

Not at all! Very glad for the feedback. 🙏

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together @ramonjd.

These sorts of improvements definitely help polish the component ✨

All in all, this tested well for me.

✅ Displays plus icon when its initial state is empty
✅ Dots icon displays when its initial state contains visible controls
✅ Icon correctly updates when controls are toggled on/off

The new plus icon looks good. At first, I wasn't sure how it might play when all the controls in the ToolsPanel have been toggled off and the user wants to "reset all". In this situation, the "reset all" option would only make a difference if it meant restoring the display of default controls. In that instance, those controls are "added" back into the panel, so the plus icon still makes some sense.

My only idea so far on how to get around this was to check the immediate children of for components that are not of type ToolsPanelItem (and therefore not registered to appear in the header toolbar).

This approach while helpful could also be a bit brittle. The ToolsPanelItem sub-component and the ToolsPanel's menu were built in such a way that a ToolsPanelItem could be wrapped or grouped in another component. This might make the check for "not of type ToolsPanelItem a little complicated.

If pursuing this we probably need to ensure we can eliminate false positives from cases such as:

const WrappedItem = ( props ) => {
	return (
		<div>
			<ToolsPanelItem { ...props }>
				<div>Just to be difficult.</div>
			</ToolsPanelItem>
		</div>
	);
};

const GroupedItems = () => {
	return (
		<>
			<ToolsPanelItem>
				<div>Grouped control</div>
			</ToolsPanelItem>
			<ToolsPanelItem>
				<div>Alt grouped control</div>
			</ToolsPanelItem>
		</>
	);
};

It's hard to see a case where a control would be added to the panel that isn't within the menu. We might add things such as help text or contrast checkers but I'm not sure that would remove the benefit of having the plus icon to encourage the user to explore the more advanced settings available through the menu.

I'll be interested to hear the thoughts of more seasoned UI gurus.

@apeatling apeatling self-requested a review August 18, 2021 16:45
Copy link
Contributor

@apeatling apeatling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this and it works as expected. I think the plus icon only when no items have been selected or are present by default is a good approach.

I think @shaunandrews might want to chime in here since he designed something with the plus menu present all of the time. Personally I'm not sure about this because it means hitting a plus button when removing/unchecking an item -- which seems strange.

Copy link
Contributor

@stacimc stacimc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good and tests well for me :)

unless it gets tripped up once the SlotFill is added, in which case it might be that the plus icon never gets shown

Just echoing this -- it looks like this might be the case? If so, I think the plus icon would never get shown for the Dimensions panel once #34063 is merged 🤔 Depending on how likely that is to land soon, maybe that changes the priority of handling the edge case. Particularly if the SlotFill isn't added to all panels, it would be odd for them to behave differently.

Otherwise, falling back to the three dots seems fine to me.

@shaunandrews
Copy link
Contributor

shaunandrews commented Aug 18, 2021

image

I tried a bunch of different icons. It might sound strange, but I almost think we could keep the chevron icon:

image

But I think it might be really confusing to use the same icon as the accordion, but open a menu instead. Though, to be fair, we use the chevron icon in many places to indicate a menu.

--

I did suggest to @apeatling that we might consider using the same icon regardless of the presence of tools. I did try having the icon change once a tool was added, but I found myself looking at a strange pattern of icons (vertical dots, , cross, horizontal dots, plus, arrow) down the right side of my browser, which felt visually unpleasant.

image

--

When there are no tools present, I think the menu should omit the "Reset all" option:

image

@ramonjd
Copy link
Member Author

ramonjd commented Aug 19, 2021

Thanks for the feedback everyone!

I think the plus icon would never get shown for the Dimensions panel once #34063 is merged 🤔 Depending on how likely that is to land soon, maybe that changes the priority of handling the edge case. Particularly if the SlotFill isn't added to all panels, it would be odd for them to behave differently.

Thanks for pointing this out. I haven't tested it on top of #34063 (if that's the direction we're taking with slots). I'll check it out and see if we can handle it.

My opinion is that this UI change is a nice to have, so I don't there's harm holding off until the ground is stable.

The ToolsPanelItem sub-component and the ToolsPanel's menu were built in such a way that a ToolsPanelItem could be wrapped or grouped in another component. This might make the check for "not of type ToolsPanelItem a little complicated.
...
It's hard to see a case where a control would be added to the panel that isn't within the menu. We might add things such as help text or contrast checkers but I'm not sure that would remove the benefit of having the plus icon to encourage the user to explore the more advanced settings available through the menu.

Thanks for confirming. I admit, it was a fairly shallow assessment. Looking at the ToolsPanels tests reveals more to me how it's intended to work.

I agree that we should be trying to help the user as much as we can to uncover hidden controls. The plus symbol to me is probably the least intrusive and least elaborate way to achieve this.

Given the small footprint of this PR, I'd be fine with merging it and then monitoring the discussion surrounding slotfills.

I'll get around to testing this on top of that branch at least soon.

I did try having the icon change once a tool was added, but I found myself looking at a strange pattern of icons (vertical dots, , cross, horizontal dots, plus, arrow) down the right side of my browser, which felt visually unpleasant.

I agree with this observation. I think icon changes might make more sense, or rather, testing icon changes makes more sense, after we migrate more of the block supports over to the ToolsPanel.

It's probably just me , but the ⚙️ symbol speaks more to me than a vertical ellipsis, but my primate brain will try to click on anything so the outcome is the same.

I suppose users won't spend too much time worrying about the content of the icon before their synapses conspire to compel the finger/hand to click on it.

@ramonjd
Copy link
Member Author

ramonjd commented Sep 13, 2021

Holding off until the changes in ToolsPanel: Refine component behaviour #34530 are finalized

@aaronrobertshaw
Copy link
Contributor

The PRs previously blocking this have been merged:

This will need a rebase. While it's being updated, would it be a good idea to add a new example to the component's storybook entry demoing the panel in an initially empty state?

@ramonjd ramonjd force-pushed the add/tools-panel-plus-icon branch from 4ff352e to ce321f2 Compare September 15, 2021 12:36
@ramonjd
Copy link
Member Author

ramonjd commented Sep 15, 2021

This will need a rebase.

Done! Thank you!

I've also tweaked the logic to only take into account optional menuItems, since I couldn't think of a better way to apply the original intent behind this PR to the new UX.

I had plans to create tests for the ToolsPanel hook, but ran out of time today.

While it's being updated, would it be a good idea to add a new example to the component's storybook entry demoing the panel in an initially empty state?

Definitely, thanks for the idea.

I did notice that the default story already displayed the plus icon, since the min-height isn't shown by default (and therefore optional)

Screen Shot 2021-09-15 at 10 31 00 pm

So I made them all display by default, and created a story specifically to showcase the plus icon.

The default now looks like this:
Screen Shot 2021-09-15 at 10 30 28 pm

And the new story:

Screen Shot 2021-09-15 at 10 40 03 pm

What do you think? I'm happy to revert it since the default story demonstrated the plus icon anyway.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had plans to create tests for the ToolsPanel hook, but ran out of time today.

This would be great. I was also thinking about having unit tests that interact with the ToolsPanel component, showing/hiding optional items and checking that the expected icon is being shown.

About the Storybook examples — what if we kept the "Minimum height" panel in the "Default" story as optional, but we made it visible (if that's even possible)? This should still hide the plus icon on the first initial render, but it will make for a more complete example (since this is the default one). Hopefully I'm making sense 😅

Also, another thing that I find useful when browsing Storybook examples is if the text in the examples describes what is being showcased. For example, the label props for each ToolsPanel and ToolsPanelItem component could read something like this:

Screenshot 2021-09-15 at 15 30 48

Screenshot 2021-09-15 at 15 32 00

Just a thought, though :)

packages/components/src/tools-panel/tools-panel/hook.js Outdated Show resolved Hide resolved
@ciampo ciampo mentioned this pull request Sep 15, 2021
62 tasks
@ramonjd
Copy link
Member Author

ramonjd commented Sep 15, 2021

Thanks for reviewing @ciampo !

Also, another thing that I find useful when browsing Storybook examples is if the text in the examples describes what is being showcased. For example, the label props for each ToolsPanel and ToolsPanelItem component could read something like this:

This was a late night copy 🍝 . Thanks for catching that one!

I was also thinking about having unit tests that interact with the ToolsPanel component, showing/hiding optional items and checking that the expected icon is being shown.

True, I looked briefly at the DOM nodes generated and I couldn't find a neat react testing library hook (role, text etc) to check the icon toggling states unless I update the aria-label to reflect the state of the icon. This is because the icon attributes, aside from the SVG attribute values, aren't unique.

Therefore I thought I'd start with the hooks before making a mess of the component tests. I'm not an expert in React Testing Library so if there are any tricks folks would like to share, I'm ready to learn!

Now I think about it, might not be a bad idea to adjust the label, at least to make it more descriptive. Currently the aria-label for the ToolsPanel icon is "Dimensions", which repeats the encasing h2 text anyway.

Screen Shot 2021-09-16 at 8 40 52 am

So maybe something like aria-label="View options" for the vertical ellipsis, and aria-label="View and add options"

I'm very certain others can come up with better labels than that! I'm just proposing the toggling of labels. 😆

@ramonjd
Copy link
Member Author

ramonjd commented Sep 15, 2021

what if we kept the "Minimum height" panel in the "Default" story as optional, but we made it visible (if that's even possible)? This should still hide the plus icon on the first initial render, but it will make for a more complete example (since this is the default one). Hopefully I'm making sense

I get it! This is a good idea, thanks. I suppose it might require sending a flag to the ToolsHeader (maybe). I'm not sure yet, but I'll check it out.

Update: I'm not sure about this. The DropDownMenu has no prop to toggle initial visibility (as far as I can see), and I can't find anything useful in the StoryBook API either, though I only looked for a minute or two so it might not count for much :)

@ramonjd ramonjd force-pushed the add/tools-panel-plus-icon branch from 2b28194 to 2984e40 Compare October 12, 2021 01:48
@ramonjd
Copy link
Member Author

ramonjd commented Oct 12, 2021

@aaronrobertshaw I think this one is ready for a final review.

Personally I don't think it's so invasive a change, and it's one we can easily roll back.

Nevertheless, I understand that there we are still working out the ToolsPanel UX and this could introduce an undesirable variable.

cc @mtias @shaunandrews for any final thoughts and preferences.

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for keeping this one up-to-date @ramonjd 👍

The new ToolsPanel menu icon works as advertised for me.

✅ Tested via storybook, block editor and global styles
ToolsPanel unit tests and type checking are all passing

I have a few minor comments or tweaks I think might be good to address.

Then I think this is good to merge as you suggested. I don't believe it will interfere with anything around updating the ToolsPanel UX.

Thanks also for your patience on this one, I appreciate it's been delayed a few times because of other ongoing work to the `ToolsPanel. 🙇

packages/components/src/tools-panel/tools-panel/hook.ts Outdated Show resolved Hide resolved
packages/components/src/tools-panel/tools-panel/hook.ts Outdated Show resolved Hide resolved
ramonjd and others added 12 commits October 13, 2021 21:12
…con to a plus symbol. This is to indicate to the user that they can add controls to the panel.
… plus icon in the ToolsPanel header.

Updated stories.
…tional items are available at all *and* if so, are they hidden
… button to describe the element and its intended effects.

Added component tests.
…roperty to the ToolsPanelContext type

Modified default value of areAllOptionalControlsHidden and updated comments
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
…anel header icon toggle in isolation from the other tests.
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
…r the state var `areAllOptionalControlsHidden`
@ramonjd ramonjd force-pushed the add/tools-panel-plus-icon branch from bc25e76 to 75e37a3 Compare October 13, 2021 10:12
@ramonjd
Copy link
Member Author

ramonjd commented Oct 13, 2021

Thanks again for giving it the expert eye @aaronrobertshaw

I've implemented the comment changes – much clearer thank you – and have addressed the rebase anomalies 😇

Then I think this is good to merge as you suggested. I don't believe it will interfere with anything around updating the ToolsPanel UX.

I think I just need an official tick or a dismissal of your last change request (I can't do it at my end) to be able to merge. Cheers!

@aaronrobertshaw aaronrobertshaw self-requested a review October 13, 2021 23:09
@aaronrobertshaw
Copy link
Contributor

I've given this a fresh test. Everything looks good to me 👍

I'll leave the honours to you :shipit:

@ramonjd ramonjd merged commit 1961b03 into trunk Oct 14, 2021
@ramonjd ramonjd deleted the add/tools-panel-plus-icon branch October 14, 2021 00:02
@github-actions github-actions bot added this to the Gutenberg 11.8 milestone Oct 14, 2021
@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants