CustomSelectControlV2: Add root element wrapper #62803

ciampo · 2024-06-24T17:08:09Z

What?

Extracted from #62424

Add a root element wrapper to CustomSelectControlV2

Why?

To fix layout bugs where outer CSS rules "leak" to the internal elements of CustomSelectControl (eg. if the parent has display: flex, which is what happens when using CustomSelectControl as a direct child of ToolsPanelItem)
For legacy reasons, in order to have a place where to apply the components-custom-select-control className

How?

Adding a root element wrapper (ie a div) instead of a React Fragment, wrapping label + button under one element.

Testing Instructions

In the editor

To test in the editor, apply this patch

Click to expand

diff --git a/packages/block-editor/src/components/font-appearance-control/index.js b/packages/block-editor/src/components/font-appearance-control/index.js
index 18e814ad23..1340396c86 100644
--- a/packages/block-editor/src/components/font-appearance-control/index.js
+++ b/packages/block-editor/src/components/font-appearance-control/index.js
@@ -1,10 +1,20 @@
 /**
  * WordPress dependencies
  */
-import { CustomSelectControl } from '@wordpress/components';
+import {
+	CustomSelectControl,
+	privateApis as componentsPrivateApis,
+} from '@wordpress/components';
 import { useMemo } from '@wordpress/element';
 import { __, _x, sprintf } from '@wordpress/i18n';
 
+/**
+ * Internal dependencies
+ */
+import { unlock } from '../../lock-unlock';
+
+const { CustomSelectControlV2Legacy } = unlock( componentsPrivateApis );
+
 const FONT_STYLES = [
 	{
 		name: _x( 'Regular', 'font style' ),
@@ -205,7 +215,7 @@ export default function FontAppearanceControl( props ) {
 
 	return (
 		hasStylesOrWeights && (
-			<CustomSelectControl
+			<CustomSelectControlV2Legacy
 				{ ...otherProps }
 				className="components-font-appearance-control"
 				label={ label }
diff --git a/packages/components/src/private-apis.ts b/packages/components/src/private-apis.ts
index f55373664e..9a26b7c7e3 100644
--- a/packages/components/src/private-apis.ts
+++ b/packages/components/src/private-apis.ts
@@ -14,6 +14,7 @@ import {
 	useCompositeStore as useCompositeStoreV2,
 } from './composite/v2';
 import { default as CustomSelectControl } from './custom-select-control';
+import { default as CustomSelectControlV2Legacy } from './custom-select-control-v2/legacy-component';
 import { positionToPlacement as __experimentalPopoverLegacyPositionToPlacement } from './popover/utils';
 import { createPrivateSlotFill } from './slot-fill';
 import {
@@ -40,6 +41,7 @@ lock( privateApis, {
 	CompositeRowV2,
 	useCompositeStoreV2,
 	CustomSelectControl,
+	CustomSelectControlV2Legacy,
 	__experimentalPopoverLegacyPositionToPlacement,
 	createPrivateSlotFill,
 	ComponentsContext,

Load the post editor
Select the block, and in the block toolbar, add the "Appearance" controls in the typography section
Notice how the label now has bottom margin

Before (trunk)	After (this PR)

github-actions · 2024-06-25T09:07:39Z

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: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>

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

ciampo · 2024-06-25T09:09:59Z

packages/components/src/custom-select-control-v2/custom-select.tsx

 		...restProps
 	} = props;

 	return (
-		<>
+		// Where should `restProps` be forwarded to?


This is something unrelated to this PR, but that we should definitely discuss.

Currently, ...restProps are assumed to be intended for CustomSelectButton, but I'd argue that folks would expect that most attributes would be assigned to the root wrapper (which will likely become BaseControl)

I think it should be the CustomSelectButton. Is there anything other than className that you'd want to pass to the root wrapper? I'm thinking about refs and event listeners, which would pass more naturally onto the button.

I'm worried that if we start adding exceptions like className, devEx will suffer because it won't be clear where each prop will be forwarded to. Also, if a consumer of the component wanted to set a className on the trigger button, how would they do it?

Another though: if we don't pass any props to the root wrapper element, do you see that as a limitation for consumers? For example,:

let's say that as a consumer I want to hide CustomSelectControl — setting display: none would only hide the button, but keep the label around

let's say I'm rendering CustomSelectControlV2 inside a flexbox parent, and I'd like to set flex-shring on it. Using className or style won't cut it, since those styles should be applied to the root wrapper. The only way to achieve that will be to add an external wrapper component to CustomSelectControlV2.

I think that this is a broader conversation that we need to have as we export components that internally compose smaller components.

For example, in DropdownMenuV2 we solved this by exporting a separate Trigger subcomponent, and exposing a trigger render prop. Which means that props by default can be passed to the root element, while trigger-specific props are passed directly to the Trigger subcomponent.

Curious to hear @DaniGuardiola 's thoughts on this one too.

mirka

Looks good 🚀

mirka · 2024-06-26T13:55:23Z

packages/components/src/custom-select-control-v2/legacy-component/index.tsx

@@ -21,6 +22,7 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) {
 		onChange,
 		size = 'default',
 		value,
+		className: classNameProp,


Why the rename?

Because I would otherwise get a 'className' is already declared in the upper scope linting error later in the file where options is mapped into <CustomSelectItem />s

mirka · 2024-06-26T14:05:10Z

packages/components/src/custom-select-control-v2/custom-select.tsx

 		...restProps
 	} = props;

 	return (
-		<>
+		// Where should `restProps` be forwarded to?


I think it should be the CustomSelectButton. Is there anything other than className that you'd want to pass to the root wrapper? I'm thinking about refs and event listeners, which would pass more naturally onto the button.

ciampo · 2024-06-26T14:46:21Z

I'm going to merge once CI cheks pass — although we should continue the conversation in #62803 (comment) and work on it via a follow-up as necessary.

ciampo self-assigned this Jun 25, 2024

ciampo changed the title ~~CustomSelectControlV2 Legacy Adapter: Add root element wrapper~~ CustomSelectControlV2: Add root element wrapper Jun 25, 2024

ciampo force-pushed the fix/custom-select-control-v2-root-wrapper branch from e29ca7a to 078a2de Compare June 25, 2024 08:49

ciampo requested a review from a team June 25, 2024 09:07

ciampo added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Jun 25, 2024

ciampo marked this pull request as ready for review June 25, 2024 09:07

ciampo requested a review from ajitbohra as a code owner June 25, 2024 09:07

ciampo commented Jun 25, 2024

View reviewed changes

This was referenced Jun 25, 2024

CustomSelectControl: refactor with ariakit #55023

Open

CustomSelectControlV2: Fix styling issues #62424

Closed

mirka approved these changes Jun 26, 2024

View reviewed changes

ciampo added 3 commits June 26, 2024 16:27

Add root element wrapper

00c33a3

Add legacy root className + ability to pass one externally

825e893

CHANGELOG

1374b58

ciampo force-pushed the fix/custom-select-control-v2-root-wrapper branch from 078a2de to 1374b58 Compare June 26, 2024 14:45

ciampo enabled auto-merge (squash) June 26, 2024 14:46

ciampo merged commit 2be17bb into trunk Jun 26, 2024
61 checks passed

ciampo deleted the fix/custom-select-control-v2-root-wrapper branch June 26, 2024 15:19

github-actions bot added this to the Gutenberg 18.8 milestone Jun 26, 2024

ciampo mentioned this pull request Jul 18, 2024

@wordpress/components: high-level API strategy for compound components #63704

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CustomSelectControlV2: Add root element wrapper #62803

CustomSelectControlV2: Add root element wrapper #62803

ciampo commented Jun 24, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024 •

edited

Loading

ciampo Jun 25, 2024 •

edited

Loading

mirka Jun 26, 2024

ciampo Jun 26, 2024 •

edited

Loading

mirka left a comment

mirka Jun 26, 2024

ciampo Jun 26, 2024 •

edited

Loading

mirka Jun 26, 2024

ciampo commented Jun 26, 2024

CustomSelectControlV2: Add root element wrapper #62803

CustomSelectControlV2: Add root element wrapper #62803

Conversation

ciampo commented Jun 24, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

In the editor

github-actions bot commented Jun 25, 2024 • edited Loading

ciampo Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

mirka Jun 26, 2024

Choose a reason for hiding this comment

ciampo Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

mirka Jun 26, 2024

Choose a reason for hiding this comment

ciampo Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

mirka Jun 26, 2024

Choose a reason for hiding this comment

ciampo commented Jun 26, 2024

ciampo commented Jun 24, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024 •

edited

Loading

ciampo Jun 25, 2024 •

edited

Loading

ciampo Jun 26, 2024 •

edited

Loading

ciampo Jun 26, 2024 •

edited

Loading