ToolbarGroup - Typescript

[margolisj]

What?

Converts ToolbarGroup to typescript.

Why?

Typescript is the only sane was to maintain a codebase this large.

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

[margolisj]

@mirka @ciampo #35744. Restarting / recovering this code from my previous attempt at ToolbarGroup from before I pulled from trunk on the wrong branch... Anyways, looking like this one is going to be a bear with how tightly Dropdown and Toolbar are integrated together.

[ciampo]

This was a tricky one, given how entangled this component is with DropdownMenu (sigh) and some legacy convoluted logic. I tried to make sense of it and left comments, let me know if that makes sense to you too!

On top of the proposed inline changes, we'll also need to tweak slightly the unit tests, updating from using strings for icons (ie. the old, deprecated dashicon syntax) to using SVG icons from @wordpress/icons) and tweaking the onClick type.

diff --git a/packages/components/src/toolbar/test/toolbar-group.tsx b/packages/components/src/toolbar/test/toolbar-group.tsx
index e37d9fdc14..b65f0e3ae9 100644
--- a/packages/components/src/toolbar/test/toolbar-group.tsx
+++ b/packages/components/src/toolbar/test/toolbar-group.tsx
@@ -3,6 +3,11 @@
  */
 import { fireEvent, render, screen } from '@testing-library/react';
 
+/**
+ * WordPress dependencies
+ */
+import { wordpress } from '@wordpress/icons';
+
 /**
  * Internal dependencies
  */
@@ -23,10 +28,10 @@ describe( 'ToolbarGroup', () => {
 		} );
 
 		it( 'should render a list of controls with buttons', () => {
-			const clickHandler = ( event: Event ) => event;
+			const clickHandler = ( event?: React.MouseEvent ) => event;
 			const controls = [
 				{
-					icon: 'wordpress',
+					icon: wordpress,
 					title: 'WordPress',
 					onClick: clickHandler,
 					isActive: false,
@@ -41,10 +46,10 @@ describe( 'ToolbarGroup', () => {
 		} );
 
 		it( 'should render a list of controls with buttons and active control', () => {
-			const clickHandler = ( event: Event ) => event;
+			const clickHandler = ( event?: React.MouseEvent ) => event;
 			const controls = [
 				{
-					icon: 'wordpress',
+					icon: wordpress,
 					title: 'WordPress',
 					onClick: clickHandler,
 					isActive: true,
@@ -63,14 +68,14 @@ describe( 'ToolbarGroup', () => {
 				[
 					// First set.
 					{
-						icon: 'wordpress',
+						icon: wordpress,
 						title: 'WordPress',
 					},
 				],
 				[
 					// Second set.
 					{
-						icon: 'wordpress',
+						icon: wordpress,
 						title: 'WordPress',
 					},
 				],
@@ -95,7 +100,7 @@ describe( 'ToolbarGroup', () => {
 			const clickHandler = jest.fn();
 			const controls = [
 				{
-					icon: 'wordpress',
+					icon: wordpress,
 					title: 'WordPress',
 					onClick: clickHandler,
 					isActive: true,

I also found myself having to tweak Toolbar, tweaking the part where props are forwarded to ToolbarGroup

diff --git a/packages/components/src/toolbar/toolbar/index.tsx b/packages/components/src/toolbar/toolbar/index.tsx
index 96e35d399d..2eac5c5e61 100644
--- a/packages/components/src/toolbar/toolbar/index.tsx
+++ b/packages/components/src/toolbar/toolbar/index.tsx
@@ -42,7 +42,15 @@ function UnforwardedToolbar(
 			alternative: 'ToolbarGroup component',
 			link: 'https://developer.wordpress.org/block-editor/components/toolbar/',
 		} );
-		return <ToolbarGroup { ...props } className={ className } />;
+		// Extracting title from `props` because `ToolbarGroup` doesn't accept it.
+		const { title: _title, ...restProps } = props;
+		return (
+			<ToolbarGroup
+				isCollapsed={ false }
+				{ ...restProps }
+				className={ className }
+			/>
+		);
 	}
 	// `ToolbarGroup` already uses components-toolbar for compatibility reasons.
 	const finalClassName = classnames(

[ciampo]

Hey @margolisj , I see some comments have been marked as resolved, but not commits were pushed to the PR. I asusme that you're still working through the feedback.

Let me know when all comments have been addressed, and I can take another look

[margolisj]

I think the types got a little wonky with how many changes there were. I'll spend a little time this afternoon going back through your comments and making sure I didn't foobar too many of them.

Dangling type error:

packages/components/src/toolbar/toolbar-group/index.tsx(103,7): error TS2322: Type '{ icon?: IconType | null | undefined; title: string; isDisabled?: boolean | undefined; onClick?: ((event?: MouseEvent<Element, MouseEvent> | undefined) => void) | undefined; ... 5 more ...; containerClassName: string | undefined; }' is not assignable to type 'Omit<Pick<BaseButtonProps & _ButtonProps & Omit<Pick<DetailedHTMLProps<ButtonHTMLAttributes<HTMLButtonElement>, HTMLButtonElement>, "key" | keyof ButtonHTMLAttributes<...>>, "as" | ... 1 more ... | keyof BaseButtonProps> & RefAttributes<...>, "hidden" | ... 264 more ... | keyof BaseButtonProps> | Pick<...>, "as" | k...'.
  Types of property 'role' are incompatible.
    Type 'string | null | undefined' is not assignable to type 'AriaRole | undefined'.
      Type 'null' is not assignable to type 'AriaRole | undefined'.

[ciampo]

Current TS issues should be fixed by:

Fixing the `role` type in the dropdown options to be more specific

diff --git a/packages/components/src/dropdown-menu/types.ts b/packages/components/src/dropdown-menu/types.ts
index aea558b73d..4781761037 100644
--- a/packages/components/src/dropdown-menu/types.ts
+++ b/packages/components/src/dropdown-menu/types.ts
@@ -1,7 +1,7 @@
 /**
  * External dependencies
  */
-import type { ReactNode } from 'react';
+import type { HTMLAttributes, ReactNode } from 'react';
 /**
  * Internal dependencies
  */
@@ -41,7 +41,7 @@ export type DropdownOption = {
 	/**
 	 * The role to apply to the option's HTML element
 	 */
-	role?: HTMLElement[ 'role' ];
+	role?: HTMLAttributes< HTMLElement >[ 'role' ];
 };
 
 type DropdownCallbackProps = {

and

Adding an `icon` prop to the ToolbarGroup props

diff --git a/packages/components/src/dropdown-menu/types.ts b/packages/components/src/dropdown-menu/types.ts
index 4781761037..7b141c97ac 100644
--- a/packages/components/src/dropdown-menu/types.ts
+++ b/packages/components/src/dropdown-menu/types.ts
@@ -13,7 +13,7 @@ import type { NavigableMenuProps } from '../navigable-container/types';
 
 export type DropdownOption = {
 	/**
-	 * The Dashicon icon slug to be shown for the option.
+	 * The icon to be shown for the option.
 	 */
 	icon?: IconProps[ 'icon' ];
 	/**
@@ -64,7 +64,7 @@ type ToggleProps = Partial<
 
 export type DropdownMenuProps = {
 	/**
-	 * The Dashicon icon slug to be shown in the collapsed menu button.
+	 * The icon to be shown in the collapsed menu button.
 	 *
 	 * @default "menu"
 	 */
diff --git a/packages/components/src/toolbar/toolbar-group/types.ts b/packages/components/src/toolbar/toolbar-group/types.ts
index a11fc2da8f..1bccdfc6f7 100644
--- a/packages/components/src/toolbar/toolbar-group/types.ts
+++ b/packages/components/src/toolbar/toolbar-group/types.ts
@@ -91,6 +91,10 @@ export type ToolbarGroupProps = ToolbarGroupPropsBase &
 				 * ARIA label for dropdown menu if is collapsed.
 				 */
 				title: string;
+				/**
+				 * The icon to be shown in the collapsed menu button.
+				 */
+				icon?: ToolbarGroupCollapsedProps[ 'icon' ];
 		  }
 	 );
 

Let's also cleanup the old, commented-out code

[ciampo]

@margolisj let's also add a CHANGELOG entry (under Internal)

[margolisj]

@margolisj let's also add a CHANGELOG entry (under Internal)

Too fast for me haha. Had just noticed. Pushed it.

[ciampo]

🚀