ToolsPanel: Trigger onDeselect/onSelect callbacks directly

[stokesman]

What?

Instead of using an effect hook to run some callbacks runs them from an event.

Why?

Simplification for maintainability. I noticed this while working on #60621.

How?

• Puts the onDeselect and onSelect callbacks into state accessible by toggleItem.
• Runs the callbacks from toggleItem which is the event handler for menu item clicks.
• Removes the effect hook that previously used for the above.

Automated tests

Currently, with the effect being responsible for triggering the callbacks, I wasn’t sure if onSelect or onDeselect might be triggered from controlled updates to an item’s value or interactions with the item’s actual control. I made some unit tests for such and according to these, it was never a thing (they pass on trunk or this branch). I'm not sure if these tests are actually worth adding.

diff --git a/packages/components/src/tools-panel/test/index.tsx b/packages/components/src/tools-panel/test/index.tsx
index 8d2c4f17a8..e40a470b06 100644
--- a/packages/components/src/tools-panel/test/index.tsx
+++ b/packages/components/src/tools-panel/test/index.tsx
@@ -797,6 +797,81 @@ describe( 'ToolsPanel', () => {
 		} );
 	} );
 
+	describe( 'menu item callbacks on item interaction', () => {
+		beforeEach( () => {
+			jest.clearAllMocks();
+		} );
+
+		const onDeselect = jest.fn();
+		const onSelect = jest.fn();
+		const ToolsPanelControllable = ( {
+			toolsPanelItemValue,
+			isOptional = false,
+		}: {
+			toolsPanelItemValue?: boolean;
+			isOptional?: boolean;
+		} ) => {
+			const itemProps = {
+				attributes: { value: toolsPanelItemValue },
+				hasValue: () => toolsPanelItemValue !== undefined,
+				label: isOptional ? 'Optional' : 'Standard',
+				onDeselect,
+				onSelect,
+			};
+
+			// The null panelId below simulates the panel prop when there
+			// are multiple blocks selected.
+			return (
+				<ToolsPanel { ...defaultProps } panelId={ null }>
+					<ToolsPanelItem { ...itemProps }>
+						<input
+							type="checkbox"
+							defaultChecked={ !! toolsPanelItemValue }
+							onChange={ ( event ) => {
+								controlValue = event.target.checked;
+							} }
+						/>
+					</ToolsPanelItem>
+				</ToolsPanel>
+			);
+		};
+
+		it( 'default item’s value changes should not trigger menu item callbacks', async () => {
+			const { rerender } = render(
+				<ToolsPanelControllable toolsPanelItemValue={ false } />
+			);
+
+			const control = screen.getByRole( 'checkbox' );
+			const user = userEvent.setup();
+			await user.click( control );
+			expect( controlProps.onSelect ).not.toHaveBeenCalled();
+			await user.click( control );
+			expect( controlProps.onDeselect ).not.toHaveBeenCalled();
+
+			rerender( <ToolsPanelControllable /> );
+			expect( controlProps.onDeselect ).not.toHaveBeenCalled();
+		} );
+
+		it( 'optional item’s value changes should not trigger menu item callbacks', async () => {
+			const { rerender } = render(
+				<ToolsPanelControllable
+					isOptional
+					toolsPanelItemValue={ false }
+				/>
+			);
+
+			const control = screen.getByRole( 'checkbox' );
+			const user = userEvent.setup();
+			await user.click( control );
+			expect( controlProps.onSelect ).not.toHaveBeenCalled();
+			await user.click( control );
+			expect( controlProps.onDeselect ).not.toHaveBeenCalled();
+
+			rerender( <ToolsPanelControllable isOptional /> );
+			expect( controlProps.onDeselect ).not.toHaveBeenCalled();
+		} );
+	} );
+
 	describe( 'grouped panel items within custom components', () => {
 		it( 'should render grouped items correctly', () => {
 			renderGroupedItemsInPanel();

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: stokesman <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[stokesman]

Not related to the purpose of the PR, this was just to reuse isRegistered instead of reevaluating the same condition.

[stokesman]

gutenberg/packages/components/src/tools-panel/tools-panel-item/hook.ts

Lines 54 to 63 in 5db7309

	// hasValue is a new function on every render, so do not add it as a
	// dependency to the useCallback hook! If needed, we should use a ref.
	// eslint-disable-next-line react-hooks/exhaustive-deps
	const hasValueCallback = useCallback( hasValue, [ panelId ] );
	// resetAllFilter is a new function on every render, so do not add it as a
	// dependency to the useCallback hook! If needed, we should use a ref.
	// eslint-disable-next-line react-hooks/exhaustive-deps
	const resetAllFilterCallback = useCallback( resetAllFilter, [ panelId ] );
	const onDeselect = useEvent( onDeselectProp );
	const onSelect = useEvent( onSelectProp );

The useEvent calls are added in 5db7309, and I just want to highlight the discrepancy with the callbacks above which are memoized by panelId, the same could be done for onDeselect and onSelect if that seems better. If so, I’d rather return all of them from a single useMemo.