CustomSelectControlV2: Handle long strings in selected value #62198

mirka · 2024-05-31T20:12:05Z

Part of #55023

What?

Correctly truncate and style long strings in the currently selected value in CustomSelectControlV2.

Why?

Long strings were not styled correctly.

Before

Testing Instructions

Compare the "With Long Labels" stories between CustomSelectControl (v1) and CustomSelectControlV2 Legacy. All size variants should look correct.

Note

You may notice that the LABEL is shifted down 1px in the v1. I looked into it and v1 is actually wrong in this case, so let's ignore. (FWIW, it will look correct in wp-admin due to a style in the forms.css stylesheet.)

Screenshots or screencast

	v1	v2 Default & Legacy
Long labels
With hints

github-actions · 2024-05-31T20:12:23Z

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: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: fullofcaffeine <fullofcaffeine@git.wordpress.org>
Co-authored-by: ciampo <mciampini@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-05-31T20:15:24Z

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

@@ -74,7 +74,7 @@ const CustomSelectButton = ( {
 			// move selection rather than open the popover
 			showOnKeyDown={ false }
 		>
-			<div>{ computedRenderSelectedValue( currentValue ) }</div>


div is unnecessary.

mirka · 2024-05-31T20:15:47Z

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

@@ -82,12 +82,12 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) {
 		);

 		return (
-			<>
+			<Styled.SelectedExperimentalHintWrapper>


Added a wrapper div to apply text truncation styles as in v1.

mirka · 2024-05-31T20:16:47Z

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

-		display: flex;
-		align-items: center;
-		justify-content: space-between;


No reason for this to be a flexbox.

Knowing past Marco, there's a chance flexbox was used to automatically trim extra white space (although it's also very possible that this is a remnant of a previous style and it's totally fine to remove it)

Probably totally fine to remove it :D

mirka · 2024-05-31T20:17:05Z

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

 		font-size: ${ CONFIG.fontSize };
+		text-align: left;


Text align was missing.

Nice catch 👍 I wonder how this didn't come up earlier.

mirka · 2024-05-31T20:18:23Z

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

+		color: ${ COLORS.theme.foreground };
 		cursor: pointer;
+		font-family: inherit;


Correct color and font-family was missing.

mirka · 2024-05-31T20:19:03Z

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

 		width: 100%;

 		&[data-focus-visible] {
 			outline: none; // handled by InputBase component
 		}

 		${ getSize() }
+		${ ! hasCustomRenderProp && truncateStyles }


We shouldn't add truncate styles when the consumer passes a custom render function.

tyxla

Working well in my testing and the code looks good 👍

Thanks for cleaning up the styles while passing through!

tyxla · 2024-06-06T08:09:17Z

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

@@ -55,40 +66,41 @@ export const Select = styled( Ariakit.Select, {
 		const sizes = {
 			compact: {
 				[ heightProperty ]: 32,
-				paddingInlineStart: space( 2 ),
-				paddingInlineEnd: space( 1 ),
+				paddingInlineStart: 8,


Is there a good reason to stop using the grid space util and hardcode the padding widths like that?

Curious about that, too.

No big reason other than that I thought it was simpler to read in context, especially when I want to add the chevronIconSize like this. (space() returns a calc()).

tyxla · 2024-06-06T08:13:50Z

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

 		font-size: ${ CONFIG.fontSize };
+		text-align: left;


Nice catch 👍 I wonder how this didn't come up earlier.

fullofcaffeine

LGTM! Thanks for working on this.

I started fixing other styling issues here: #62424 (it's been split from #), so I'll rebase from this, review the current issues, and continue working there.

# Conflicts: # packages/components/CHANGELOG.md

mirka added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels May 31, 2024

mirka self-assigned this May 31, 2024

mirka requested a review from ajitbohra as a code owner May 31, 2024 20:12

mirka added 3 commits June 1, 2024 05:13

CustomSelectControlV2: Handle long strings in selected value

65e3a00

Add truncate styles to custom render with hint

a68fce4

Add changelog

4747d78

mirka force-pushed the custom-select-v2-long-strings branch from f3b0f8d to a68fce4 Compare May 31, 2024 20:14

mirka commented May 31, 2024

View reviewed changes

mirka requested review from fullofcaffeine and a team May 31, 2024 20:20

mirka mentioned this pull request May 31, 2024

Components: Replace/old custom select control with v2 legacy adapter #61272

Closed

tyxla approved these changes Jun 6, 2024

View reviewed changes

fullofcaffeine approved these changes Jun 7, 2024

View reviewed changes

fullofcaffeine mentioned this pull request Jun 7, 2024

CustomSelectControlV2: Fix styling issues #62424

Closed

2 tasks

This was referenced Jun 25, 2024

CustomSelectControl: refactor with ariakit #55023

Open

CustomSelectControlV2: tweak item inline padding based on size #62850

Merged

Merge branch 'trunk' into custom-select-v2-long-strings

b6604c4

# Conflicts: # packages/components/CHANGELOG.md

mirka merged commit 7bca2fa into trunk Jun 25, 2024
62 checks passed

mirka deleted the custom-select-v2-long-strings branch June 25, 2024 17:47

github-actions bot added this to the Gutenberg 18.7 milestone Jun 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CustomSelectControlV2: Handle long strings in selected value #62198

CustomSelectControlV2: Handle long strings in selected value #62198

mirka commented May 31, 2024 •

edited

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

mirka May 31, 2024

mirka May 31, 2024

mirka May 31, 2024

ciampo Jun 10, 2024 •

edited

Loading

ciampo Jun 25, 2024

mirka May 31, 2024

tyxla Jun 6, 2024

mirka May 31, 2024

mirka May 31, 2024

tyxla left a comment

tyxla Jun 6, 2024

fullofcaffeine Jun 7, 2024

mirka Jun 25, 2024

tyxla Jun 6, 2024

fullofcaffeine left a comment

CustomSelectControlV2: Handle long strings in selected value #62198

CustomSelectControlV2: Handle long strings in selected value #62198

Conversation

mirka commented May 31, 2024 • edited Loading

What?

Why?

Before

Testing Instructions

Screenshots or screencast

github-actions bot commented May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo Jun 10, 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

Choose a reason for hiding this comment

tyxla left a 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

Choose a reason for hiding this comment

fullofcaffeine left a comment

Choose a reason for hiding this comment

mirka commented May 31, 2024 •

edited

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

ciampo Jun 10, 2024 •

edited

Loading