CustomSelectControlV2: fix item styles #62825

ciampo · 2024-06-25T11:24:03Z

What?

Part of #55023

Fix items (options) styles of CustomSelectControlV2 component

Why?

To fix visual bugs
To match more closely look of V1 component
To add quality of life improvements

How?

Update V2 items in order to match the same line height of the V1 version
Add disabled styles
Added user-select: none to trigger button and option items
Added scroll-margin to improve scroll experience when selecting items that are out of view in the popover

Testing Instructions

Compare V1 and V2 (legacy) of CustomSelectControl, make sure that the items in the select popover now render with the same line height (28px).
Try to select with the cursor the text of the items in the select popover — V2 should prevent text selection

V1	V2 before (trunk)	V2 after (this PR)

github-actions · 2024-06-25T11:27:03Z

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.

ciampo · 2024-06-25T11:28:26Z

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

@@ -108,7 +108,14 @@ export const SelectItem = styled( Ariakit.SelectItem )`
 	justify-content: space-between;
 	padding: ${ ITEM_PADDING };
 	font-size: ${ CONFIG.fontSize };
-	line-height: 2.15rem; // TODO: Remove this in default but keep for back-compat in legacy


To match 28px, the line height value should have been 2.15 (without the rem unit). I felt like a more explicit calc syntax would help to understand the reason for an arbitrary number.

It's curious where the 2.15 came from originally...

It's the ratio of the line height (28px) to the font size (13px)

Also, since 28 / 13 = 2.15384615, another consequence of using 2.15 is that the resulting line height wouldn't be exactly 28px (as per design), but slightly less because of the lower decimal precision.

In short: I prefer to let the browser (or sass) do the calculation, and if not possible, to state the line-height directly.

Also, since 28 / 13 = 2.15384615, another consequence of using 2.15 is that the resulting line height wouldn't be exactly 28px (as per design), but slightly less because of the lower decimal precision.

Which also makes me question if 28/13 was the intent. But yes, having a clear calc there makes things much clearer.

mirka · 2024-06-28T18:24:49Z

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

-	line-height: 2.15rem; // TODO: Remove this in default but keep for back-compat in legacy
+	// Legacy line height is 28px.
+	// TODO: reassess for non-legacy v2
+	line-height: calc( 28px / ${ CONFIG.fontSize } );


Apparently this calc doesn't work 😕 ("For /, the right operand must be unitless".)

Maybe if we wrap CONFIG.fontSize() in a stringToNumber()? I appreciate the clarity that the explicit calc adds.

Sometimes less is more — we can also just remove the ${ CONFIG.fontSize } part, and simply state 28px — pushed in 9a5d26f

But these 2 aren't the same, are they:

font-size: 13px; line-height: 2.15384615

and:

font-size: 13px line-height: 28px

because if someone overrides only the font-size, the line-height will behave differently - considering the new font-size in the first example, and ignoring it in the second one.

To adhere more closely to legacy behavior, It's actually better to keep the hardcoded 28px as V1 does, exactly for the reason you state. For example, in the default Storybook example of CustomSelectControl each select item has a custom font-size, but the line-height is supposed to stay the same instead of scaling up/down.

the line-height is supposed to stay the same instead of scaling up/down.

Interesting - is this always the case? What if I have font-size 56px, won't it be ugly with line-height of 28px?

I guess we can review that when assessing what line-height to adopt for the V2 — keeping it as hardcoded 28px is probably the safest choice while swapping the V1 implementation

mirka · 2024-06-28T18:27:42Z

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

+	&[aria-disabled='true'] {
+		opacity: 0.5;
+	}


I actually omitted the disabled styles on purpose, because the item styles are fully customizable and the appropriate disabled style will vary. For example, adding a disabled to the "Purple" item in the Default story doesn't result in an understandable visual.

gutenberg/packages/components/src/custom-select-control-v2/types.ts

Lines 196 to 202 in 9a331f1

/**

* If true, the item will be disabled.

*

* You will need to add your own styles (e.g. reduced opacity) to visually show that they are disabled.

* @default false

*/

disabled?: boolean;

What do you think?

You raise a good point — although I also think that the component should offer some good defaults.

Testing a middle ground in 37ee79c , using cursor: not-allowed as a more un-opinionated way to apply default disabled item styles.

Let me know if you still think this is too much

packages/components/CHANGELOG.md

tyxla · 2024-07-01T11:28:39Z

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

@@ -108,7 +108,14 @@ export const SelectItem = styled( Ariakit.SelectItem )`
 	justify-content: space-between;
 	padding: ${ ITEM_PADDING };
 	font-size: ${ CONFIG.fontSize };
-	line-height: 2.15rem; // TODO: Remove this in default but keep for back-compat in legacy


It's curious where the 2.15 came from originally...

tyxla · 2024-07-01T11:28:53Z

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

-	line-height: 2.15rem; // TODO: Remove this in default but keep for back-compat in legacy
+	// Legacy line height is 28px.
+	// TODO: reassess for non-legacy v2
+	line-height: calc( 28px / ${ CONFIG.fontSize } );


Maybe if we wrap CONFIG.fontSize() in a stringToNumber()? I appreciate the clarity that the explicit calc adds.

tyxla

Left a minor suggestion, but other than that, this looks good to me. Thanks!

tyxla · 2024-07-01T14:07:48Z

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

@@ -133,7 +134,15 @@ export const SelectItem = styled( Ariakit.SelectItem )`
 	justify-content: space-between;
 	padding: ${ ITEM_PADDING };
 	font-size: ${ CONFIG.fontSize };
-	line-height: 2.15rem; // TODO: Remove this in default but keep for back-compat in legacy
+	// TODO: reassess line-height for non-legacy v2
+	line-height: 28px;


I'd prefer 2.15384615 and adding a comment about how it's calculated, because that way we'll keep the flexibility/relativity in case someone modifies only the font-size.

See #62825 (comment)

* Fix line height * Add disabled styles * CHANGELOG * Prevent user selection for items text * Apply more tweaks * Remove extra changelog entry * Simplify line-height styles * Less opinionated item disabled styles --- 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 mentioned this pull request Jun 25, 2024

CustomSelectControl: refactor with ariakit #55023

Open

85 tasks

ciampo self-assigned this Jun 25, 2024

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

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

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

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

ciampo commented Jun 25, 2024

View reviewed changes

ciampo mentioned this pull request Jun 25, 2024

CustomSelectControlV2: Fix styling issues #62424

Closed

2 tasks

ciampo force-pushed the fix/custom-select-control-v2-item-style branch 3 times, most recently from 6ecc280 to 6235497 Compare June 27, 2024 08:52

mirka reviewed Jun 28, 2024

View reviewed changes

tyxla reviewed Jul 1, 2024

View reviewed changes

ciampo force-pushed the fix/custom-select-control-v2-item-style branch from 6235497 to 37ee79c Compare July 1, 2024 12:34

ciampo requested review from mirka and tyxla July 1, 2024 12:36

tyxla approved these changes Jul 1, 2024

View reviewed changes

ciampo enabled auto-merge (squash) July 1, 2024 16:18

ciampo added 7 commits July 1, 2024 18:25

Fix line height

0d8c8a1

Add disabled styles

f04567f

CHANGELOG

eccd3cd

Prevent user selection for items text

ad808c5

Apply more tweaks

0820b99

Simplify line-height styles

1ec1750

Less opinionated item disabled styles

ee95320

ciampo force-pushed the fix/custom-select-control-v2-item-style branch from 37ee79c to ee95320 Compare July 1, 2024 16:26

ciampo merged commit 12518a0 into trunk Jul 1, 2024
62 checks passed

ciampo deleted the fix/custom-select-control-v2-item-style branch July 1, 2024 17:01

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CustomSelectControlV2: fix item styles #62825

CustomSelectControlV2: fix item styles #62825

ciampo commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024 •

edited

Loading

ciampo Jun 25, 2024

tyxla Jul 1, 2024

ciampo Jul 1, 2024

tyxla Jul 1, 2024

mirka Jun 28, 2024

tyxla Jul 1, 2024

ciampo Jul 1, 2024 •

edited

Loading

tyxla Jul 1, 2024

ciampo Jul 1, 2024 •

edited

Loading

tyxla Jul 1, 2024

ciampo Jul 1, 2024

mirka Jun 28, 2024

ciampo Jul 1, 2024 •

edited

Loading

tyxla Jul 1, 2024

tyxla Jul 1, 2024

tyxla left a comment

tyxla Jul 1, 2024

ciampo Jul 1, 2024

	/**
	* If true, the item will be disabled.
	*
	* You will need to add your own styles (e.g. reduced opacity) to visually show that they are disabled.
	* @default false
	*/
	disabled?: boolean;

CustomSelectControlV2: fix item styles #62825

CustomSelectControlV2: fix item styles #62825

Conversation

ciampo commented Jun 25, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

github-actions bot commented Jun 25, 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

ciampo Jul 1, 2024 • edited Loading

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

Choose a reason for hiding this comment

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

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

ciampo commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024 •

edited

Loading

ciampo Jul 1, 2024 •

edited

Loading

ciampo Jul 1, 2024 •

edited

Loading

ciampo Jul 1, 2024 •

edited

Loading