CustomSelectControl V2: allow wrapping item hint to new line #62848

ciampo · 2024-06-25T16:04:31Z

What?

Improve how inidivual item hints are rendered for the CustomSelectControl V2 legacy adapter, by allowing the hint to wrap below the item's content, instead of being forced on the same line.

Why?

Even if the previous layout is how the V1 component behaves, it does not produce good-looking UI when the select popover is narrow. And it often overridden in the editor via custom style overrides anyway, which is bad.

These changes basically bring to the component the overrides that were applied in the editor.

How?

Using flexbox to allow the hint to break the line and render below the item's content.

Testing Instructions

Storybook

Apply the following patch

Click to show diff

diff --git a/packages/components/src/custom-select-control/stories/index.story.tsx b/packages/components/src/custom-select-control/stories/index.story.tsx
index 8ff9a023c5..4636fd34d3 100644
--- a/packages/components/src/custom-select-control/stories/index.story.tsx
+++ b/packages/components/src/custom-select-control/stories/index.story.tsx
@@ -111,7 +111,8 @@ WithHints.args = {
 		{
 			key: 'medium',
 			name: 'Medium',
-			__experimentalHint: '250x250',
+			__experimentalHint:
+				'A long hint, which ultimately could have been just "250x250"',
 		},
 		{
 			key: 'large',

Load the "With Hints" storybook example for the V2 legacy adapter
Resize the window and check the new hint wrapping behaviour
Compare it with the same storybook example for the V1 component

V1	V2 legacy adapter - before (trunk)	V2 legacy adapter - after (this PR)
hint.-storybook.-.cscv1.mp4	hint.-storybook.-.cscv2.before.mp4	hint.-storybook.-.cscv2.after.mp4

Editor

First, apply the following patch

Click to show diff

diff --git a/packages/block-editor/src/components/date-format-picker/index.js b/packages/block-editor/src/components/date-format-picker/index.js
index 19ec0bf8c2..f71cdd5e48 100644
--- a/packages/block-editor/src/components/date-format-picker/index.js
+++ b/packages/block-editor/src/components/date-format-picker/index.js
@@ -8,11 +8,18 @@ import {
 	TextControl,
 	ExternalLink,
 	VisuallyHidden,
-	CustomSelectControl,
 	ToggleControl,
 	__experimentalVStack as VStack,
+	privateApis as componentsPrivateApis,
 } from '@wordpress/components';
 
+/**
+ * Internal dependencies
+ */
+import { unlock } from '../../lock-unlock';
+
+const { CustomSelectControlV2Legacy } = unlock( componentsPrivateApis );
+
 // So that we illustrate the different formats in the dropdown properly, show a date that is
 // somwhat recent, has a day greater than 12, and a month with more than three letters.
 const exampleDate = new Date();
@@ -128,7 +135,7 @@ function NonDefaultControls( { format, onChange } ) {
 
 	return (
 		<VStack>
-			<CustomSelectControl
+			<CustomSelectControlV2Legacy
 				label={ __( 'Choose a format' ) }
 				options={ [ ...suggestedOptions, customOption ] }
 				value={
diff --git a/packages/block-editor/src/components/date-format-picker/style.scss b/packages/block-editor/src/components/date-format-picker/style.scss
index d1ad52408d..748e43bb8d 100644
--- a/packages/block-editor/src/components/date-format-picker/style.scss
+++ b/packages/block-editor/src/components/date-format-picker/style.scss
@@ -4,13 +4,4 @@
 
 .block-editor-date-format-picker__custom-format-select-control__custom-option {
 	border-top: 1px solid $gray-300;
-
-	&.has-hint {
-		grid-template-columns: auto 30px;
-	}
-
-	.components-custom-select-control__item-hint {
-		grid-row: 2;
-		text-align: left;
-	}
 }
diff --git a/packages/block-editor/src/hooks/position.js b/packages/block-editor/src/hooks/position.js
index a5d4a0cb43..b184afca46 100644
--- a/packages/block-editor/src/hooks/position.js
+++ b/packages/block-editor/src/hooks/position.js
@@ -26,7 +26,9 @@ import { cleanEmptyObject, useStyleOverride } from './utils';
 import { unlock } from '../lock-unlock';
 import { store as blockEditorStore } from '../store';
 
-const { CustomSelectControl } = unlock( componentsPrivateApis );
+const { CustomSelectControl, CustomSelectControlV2Legacy } = unlock(
+	componentsPrivateApis
+);
 
 const POSITION_SUPPORT_KEY = 'position';
 
@@ -287,7 +289,7 @@ export function PositionPanelPure( {
 						__nextHasNoMarginBottom
 						help={ stickyHelpText }
 					>
-						<CustomSelectControl
+						<CustomSelectControlV2Legacy
 							__next40pxDefaultSize
 							className="block-editor-hooks__position-selection__select-control"
 							label={ __( 'Position' ) }
diff --git a/packages/block-editor/src/hooks/position.scss b/packages/block-editor/src/hooks/position.scss
index b3bd6b1b9e..552ee8cf7c 100644
--- a/packages/block-editor/src/hooks/position.scss
+++ b/packages/block-editor/src/hooks/position.scss
@@ -3,16 +3,3 @@
 		display: none;
 	}
 }
-
-.block-editor-hooks__position-selection__select-control__option {
-	&.has-hint {
-		grid-template-columns: auto 30px;
-		line-height: $default-line-height;
-		margin-bottom: 0;
-	}
-
-	.components-custom-select-control__item-hint {
-		grid-row: 2;
-		text-align: left;
-	}
-}
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,

DateFormat picker

In the post editor, add a "Posts List" block
Pick a template which features the post data (most do)
Select the post date block, and in the sidebar interact with the date format (you may have to toggle off the "Default format" toggle)
Scroll the list of date format options all the way to the bottom, where the "Custom" option is
Make sure that the layout matches what's currently on trunk

V1	V2 legacy adapter - before (trunk)	V2 legacy adapter - after (this PR)
hint.-.date.format.-.cscv1.mp4	hint.-.date.format.-.cscv2.before.mp4	hint.-.date.format.-.cscv2.after.mp4

Position picker

In the post editor, add a "Group" block
Select the group block, and in the sidebar expand the "Position" panel
Open the position select, and notice the Sticky option
Make sure that the layout matches what's currently on trunk

V1	V2 legacy adapter - before (trunk)	V2 legacy adapter - after (this PR)
hint.-.position.-.cscv1.mp4	hint.-.position.-.cscv2.before.mp4	hint.-.position.-.cscv2.after.mp4

Known bugs

There are several known layout bugs related to CustomSelectControlV2 — most of them are addressed in separate PRs (see #55023). When reviewing this PR, please focus only on the layout of option item hints.

ciampo · 2024-06-26T05:59:11Z

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

+				<Styled.ItemHintWrapper>
+					<Styled.ItemHintContent>{ name }</Styled.ItemHintContent>
+					<Styled.ItemHint className="components-custom-select-control__item-hint">
 						{ __experimentalHint }
-					</Styled.ExperimentalHintItem>
-				</Styled.WithHintWrapper>
+					</Styled.ItemHint>
+				</Styled.ItemHintWrapper>


Notable changes:

Converted the span that wraps { name } to a styled component to add some padding-inline-end, ensuring a bit of spacing between the item content and the hint

Renamed the remaining styled components to have a more coherent name structure

The name ItemHintContent feels wrong to me, since it wraps the item content and not the hint content.

Also, did we consider adding a gap instead of padding?

The name ItemHintContent feels wrong to me, since it wraps the item content and not the hint content.

I wasn't happy with it either but I've been struggling with good naming — pushed a new attempt, hopefully things are more clear!

Also, did we consider adding a gap instead of padding?

I initially considered using gap, but then realised that I needed different gaps based on whether the hint was rendering on the same line, or wrapping on the line line — and therefore I opted for padding and margin.

But after your question, I looked into it again, and discovered and row-gap and column-gap apply to flexbox too, and so I refactored accordingly — thank you for the suggestion!

ciampo · 2024-06-26T06:01:43Z

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

+						'components-custom-select-control__item',
+						!! __experimentalHint && 'has-hint',


Consumers of the V1 component expect these classnames and use them to style specifically the hint part (try searching for .has-hint in the codebase).

I'll see if I can bring in more changes to the hint so that these overrides are not necessary in the editor, but it's probably sensible to leave those classnames there, at least for the legacy adapter.

github-actions · 2024-06-26T06:25:53Z

Flaky tests detected in dfcedd9.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9674369680
📝 Reported issues:

[Flaky Test] submenu click on the arrow interactions #61884 in /test/e2e/specs/editor/blocks/navigation-frontend-interactivity.spec.js

packages/components/src/custom-select-control-v2/styles.ts

ciampo · 2024-06-26T09:42:55Z

packages/components/src/custom-select-control-v2/styles.ts

+	align-items: center;
+	flex-wrap: wrap;


Added these two rules to allow hint to wrap, and to preserve vertical centering even after adding a smaller line-height for the hint

ciampo · 2024-06-26T09:43:19Z

packages/components/src/custom-select-control-v2/styles.ts

+		color: ${ COLORS.theme.gray[ 600 ] };
+		text-align: initial;
+		line-height: 1.4;
+		padding-inline-end: ${ space( 1 ) };


Using padding here to better match legacy behavior. Also, the V1 styles are loaded in the editor, if we use the legacy components-custom-select-control__item we will get those styles applied to the V2 as well 😅 Without changing margin-inline-end to padding-inline-end, the hint would get both a margin and a padding applied

ciampo · 2024-06-26T09:45:58Z

packages/components/src/custom-select-control-v2/styles.ts

+	// '.components-custom-select-control__item-hint' class
+	${ SelectItem } && {
+		color: ${ COLORS.theme.gray[ 600 ] };
+		text-align: initial;


Since we use flexbox to push the hint to the right, text-align isn't necessary per se, but since V1 styles are "leaking" to this implementation too, we need to reset the value to initial. This allows the hint text to be correctly aligned both when it's on the same line as the item's content, and when it wraps to its own independent line.

ciampo · 2024-06-26T09:49:11Z

packages/components/src/custom-select-control-v2/styles.ts

+	${ SelectItem } && {
+		color: ${ COLORS.theme.gray[ 600 ] };
+		text-align: initial;
+		line-height: ${ CONFIG.fontLineHeightBase };


The hint needs to use the default line-height value (and not the one of the option item), since it could render multi-line text. When rendered on the same line as the option item text, vertical alignment is still guaranteed by flexbox.

packages/components/src/custom-select-control-v2/styles.ts

github-actions · 2024-06-26T11:19:19Z

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 <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

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

mirka · 2024-06-29T12:32:52Z

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

+				<Styled.ItemHintWrapper>
+					<Styled.ItemHintContent>{ name }</Styled.ItemHintContent>
+					<Styled.ItemHint className="components-custom-select-control__item-hint">
 						{ __experimentalHint }
-					</Styled.ExperimentalHintItem>
-				</Styled.WithHintWrapper>
+					</Styled.ItemHint>
+				</Styled.ItemHintWrapper>


The name ItemHintContent feels wrong to me, since it wraps the item content and not the hint content.

Also, did we consider adding a gap instead of padding?

mirka · 2024-06-29T12:39:54Z

packages/components/src/custom-select-control-v2/styles.ts

+	// Necessary to override V1 styles passed via the legacy
+	// '.components-custom-select-control__item' class


Do you mean these styles? I guess we can remove the && hack right after we replace the v1 with v2 legacy, but also we might just want to remove the v1 stylesheet import while we test v2 legacy styling because there are probably other weird interference as well.

Do you mean these styles?

Yup!

I guess we can remove the && hack right after we replace the v1 with v2 legacy

That's what I had in mind as well — I've added an extra item in the tracking issue so that we don't forget

we might just want to remove the v1 stylesheet import while we test v2 legacy styling because there are probably other weird interference as well.

That's a good point. Let's schedule some testing with/without importing V1 styles once all current style fixes are merged — added that to the tracking issues too.

Another way I just thought of is to comment out the legacy classnames we're passing in legacy-component/index.tsx until we're ready to remove the v1 implementation and styles.

Your call, but that might be simpler than cleaning up the small hacks we're adding in this PR.

Clever! I went ahead and followed your suggestion — seems to work well!

When gradually swapping to legacy V2 we'll probably have to enable them (at least while testing) so that overrides like this one can take effect — unless we ignore those overrides and have the component behave "vanilla".

mirka

Looks good and works well, though this might be an alternative to merging in some of the temporary hacks.

tyxla

Looking good and testing well for me 👍

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

…v1 styles

…g when on separate rows

…ss#62848) * Wrap item hint when it doesn't fit * Set more legacy classnames * Use padding instead of margin * CHANGELOG * Move hint styles to bottom of file, add more specificity to override v1 styles * Better defaults for V2 legacy styles * Use line-height variable * Add block margin to hint for better spacing when collapsed * More renaming * Today I (re-)learned row and column gap * Comment out legacy classnames, remove && specificity hack in styles * Swap row-gap with margin-block on the hint for better vertical spacing when on separate rows --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>

ciampo changed the title ~~Add minimum width to select popover~~ CustomSelectControl V2: allow wrapping item hint to new line Jun 25, 2024

This was referenced Jun 25, 2024

CustomSelectControl: refactor with ariakit #55023

Open

CustomSelectControlV2: Fix styling issues #62424

Closed

ciampo force-pushed the fix/custom-select-control-hint-wrapping branch from 1430782 to 57b3613 Compare June 26, 2024 05:55

ciampo commented Jun 26, 2024

View reviewed changes

packages/components/src/custom-select-control-v2/styles.ts Show resolved Hide resolved

ciampo commented Jun 26, 2024

View reviewed changes

packages/components/src/custom-select-control-v2/styles.ts Outdated Show resolved Hide resolved

ciampo marked this pull request as ready for review June 26, 2024 11:19

ciampo requested a review from ajitbohra as a code owner June 26, 2024 11:19

ciampo requested a review from a team June 26, 2024 11:19

ciampo self-assigned this Jun 26, 2024

ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Jun 26, 2024

ciampo force-pushed the fix/custom-select-control-hint-wrapping branch from a45f844 to 9a60dab Compare June 27, 2024 08:19

mirka reviewed Jun 29, 2024

View reviewed changes

ciampo force-pushed the fix/custom-select-control-hint-wrapping branch from 9a60dab to 21950ec Compare July 1, 2024 12:13

ciampo requested a review from mirka July 1, 2024 12:23

mirka approved these changes Jul 1, 2024

View reviewed changes

tyxla approved these changes Jul 2, 2024

View reviewed changes

packages/components/src/custom-select-control-v2/legacy-component/index.tsx Outdated Show resolved Hide resolved

packages/components/src/custom-select-control-v2/legacy-component/index.tsx Outdated Show resolved Hide resolved

ciampo added 4 commits July 2, 2024 16:10

Wrap item hint when it doesn't fit

6282acf

Set more legacy classnames

b7d76eb

Use padding instead of margin

a051464

CHANGELOG

8d86863

ciampo added 6 commits July 2, 2024 16:12

Move hint styles to bottom of file, add more specificity to override …

ee1af42

…v1 styles

Better defaults for V2 legacy styles

af778f0

Use line-height variable

45725a0

Add block margin to hint for better spacing when collapsed

72f284d

More renaming

272e9f9

Today I (re-)learned row and column gap

7fa19ff

ciampo force-pushed the fix/custom-select-control-hint-wrapping branch from 21950ec to 92e358b Compare July 2, 2024 14:50

ciampo added 2 commits July 2, 2024 16:51

Comment out legacy classnames, remove && specificity hack in styles

524fa0c

Swap row-gap with margin-block on the hint for better vertical spacin…

48f1a0d

…g when on separate rows

ciampo force-pushed the fix/custom-select-control-hint-wrapping branch from 92e358b to 48f1a0d Compare July 2, 2024 14:51

ciampo enabled auto-merge (squash) July 2, 2024 14:53

ciampo merged commit 5338926 into trunk Jul 2, 2024
61 checks passed

ciampo deleted the fix/custom-select-control-hint-wrapping branch July 2, 2024 15:27

github-actions bot added this to the Gutenberg 18.8 milestone Jul 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CustomSelectControl V2: allow wrapping item hint to new line #62848

CustomSelectControl V2: allow wrapping item hint to new line #62848

ciampo commented Jun 25, 2024 •

edited

Loading

ciampo Jun 26, 2024

mirka Jun 29, 2024

ciampo Jul 1, 2024 •

edited

Loading

ciampo Jun 26, 2024 •

edited

Loading

github-actions bot commented Jun 26, 2024

ciampo Jun 26, 2024 •

edited

Loading

ciampo Jun 26, 2024

ciampo Jun 26, 2024

ciampo Jun 26, 2024

github-actions bot commented Jun 26, 2024 •

edited

Loading

mirka Jun 29, 2024

mirka Jun 29, 2024

ciampo Jul 1, 2024

mirka Jul 1, 2024

ciampo Jul 2, 2024

mirka left a comment

tyxla left a comment

		'components-custom-select-control__item',
		!! __experimentalHint && 'has-hint',

		// Necessary to override V1 styles passed via the legacy
		// '.components-custom-select-control__item' class

CustomSelectControl V2: allow wrapping item hint to new line #62848

CustomSelectControl V2: allow wrapping item hint to new line #62848

Conversation

ciampo commented Jun 25, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Storybook

Editor

DateFormat picker

Position picker

Known bugs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

ciampo Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jun 26, 2024

ciampo Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

ciampo commented Jun 25, 2024 •

edited

Loading

ciampo Jul 1, 2024 •

edited

Loading

ciampo Jun 26, 2024 •

edited

Loading

ciampo Jun 26, 2024 •

edited

Loading

github-actions bot commented Jun 26, 2024 •

edited

Loading