CustomSelectControlV2: Use `InputBase` for styling #60261

mirka · 2024-03-27T21:39:39Z

What?

Reuse InputBase and SelectControlChevronDown for styling CustomSelectControlV2.

Why?

For more DRY and consistent styling.

(InputBase is an internal component used to style the standard borders for an input, as well as handle the layout for prefix/suffix elements.)

Testing Instructions

Focus styles for CustomSelectControlV2 Legacy should match the styles for v1.
The chevron should have the correct padding in all possible sizes.

Note

Long strings will still overlap with the chevron. I'll address this separately.

mirka · 2024-03-27T21:41:30Z

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

-const CustomSelectButton = contextConnectWithoutRef(
-	UnconnectedCustomSelectButton,
-	'CustomSelectControlButton'
-);


We don't need to connect this to the context system anymore.

Would you mind elaborating on why?

Nevermind, already seen #60261 (comment) 👍🏻

mirka · 2024-03-27T21:45:09Z

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

-	const translatedProps = {
-		'aria-describedby': props.describedBy,
-		children,
-		renderSelectedValue: __experimentalShowSelectedHint
-			? renderSelectedValueHint
-			: undefined,
-		...restProps,
-	};


I replaced this with direct prop assignments on the _CustomSelect component for simplicity.

mirka · 2024-03-27T21:46:43Z

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

-	}, [ __next40pxDefaultSize, size ] );
-
-	const translatedProps = {
-		'aria-describedby': props.describedBy,


describedBy wasn't being destructured from the props, and therefore it was still being passed invalidly to _CustomSelect through the rest props. Fixed.

mirka · 2024-03-27T21:47:45Z

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

 		}
-
-		return {
-			CustomSelectControlButton: { _overrides: { size: selectedSize } },


No more need to tunnel this via context system, because we can directly set the newly exposed size prop on _CustomSelect.

Ah I see 👍🏻 (regarding https://github.com/WordPress/gutenberg/pull/60261/files#r1616307373).

mirka · 2024-03-27T21:51:10Z

packages/components/src/input-control/input-base.tsx

@@ -64,7 +64,7 @@ function getUIFlexProps( labelPosition?: LabelPosition ) {
 	return props;
 }

-export function InputBase(


Removed this unused export, because a consumer can erroneously import this named version instead of the default export which is properly wrapped in a forwardRef().

(This happened to me while working on this PR and it wasted a couple minutes of my life 😇)

Nit, but, what is the current convention for components - should we provide both (named, default) or just one - and if so, it seems default is preferred?

Another approach would be to connect before while assigning to a const and then export as needed.

Good question. We do indeed have some conventions that are actually for accommodating our current docgen, more than anything else.

Doesn't matter that much for internal components like InputBase, which aren't fed into the docgen.

mirka · 2024-03-27T21:54:37Z

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

I refactored some of these types because the rejiggering work I did in this PR surfaced some mistakes in how we were using them.

mirka · 2024-05-24T13:07:38Z

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

@@ -14,6 +14,19 @@ export type CustomSelectStore = {

 export type CustomSelectContext = CustomSelectStore | undefined;

+type CustomSelectSize< Size = 'compact' | 'default' > = {


For context, the public interface of CustomSelectControlV2 only has these two size variants, whereas the private (internal) CustomSelectButton has three size variants for back compat.

github-actions · 2024-05-24T13:29:14Z

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: fullofcaffeine <fullofcaffeine@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>

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

fullofcaffeine

I didn't know about InputBase; I can see it's part of the input-control component. Would it make more sense to make it a separate component if it's going to be used as the basis for others like this (in a follow-up, ofc)? It feels weird it doesn't have its own doc, too. For now, it'd be nice if you could add a bit more information/background on InputBase in the PR description, if possible.

Other than that, the changes looks good! Storybook stories also look good. However, I can still see the API and styling issues I've found while working on #61272. Since this component is still private, I think it's fine to merge this now and rebase/merge it into #61272 to see how it behaves with the fixes there. Will start looking into it as soon as this is merged.

tyxla

Looks good to me visually and works well 👍 Thanks.

Just left a couple of minor questions, but other than that, good to go 🚀

tyxla · 2024-05-28T10:36:10Z

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

-	const contextSystemValue = useMemo( () => {
-		let selectedSize;
-
+	const translatedSize = ( () => {


Is there a good reason for the IIFE here? Can be a simple if without an IIFE IMHO.

To me, this takes the least cognitive load to read because it doesn't introduce a mutable variable (let), and it's a bit of overkill to extract out the entire function. I'm probably more mutation-averse than average though 😓

Primitive values cannot be mutated, so we're technically not really mutating anything. Also, the IIFE makes the code a bit more complex to read than if we were using a let. No strong feelings either way, so I'm happy to leave this to your judgment!

Touché, I should've said reassignment! I always considered variable reassignment to be a form of mutation (as in mutable state), but I guess those two terms should be differentiated in JS at least.

tyxla · 2024-05-28T10:47:22Z

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

+			}
+			size={ translatedSize }
+			store={ store }
+			{ ...restProps }


Do we intentionally allow the restProps to override the above props like store or renderSelectedValue? Should these props be declared below the restProps to prevent that from happening?

Everything that can't be merged should be overridable, as per the open/closed pattern.

Also store or renderSelectedValue are not documented in the public interface for this, so it would be very at-your-own-risk if anyone tried that.

No strong opinion, but I think the general strategy of "keep everything overridable unless there is a specific, strong reason not to" would be fine.

Would be much better if JS supported if expressions :/

github-actions · 2024-05-28T15:06:42Z

Flaky tests detected in 106f28a.
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/9271457879
📝 Reported issues:

[Flaky Test] Items list is reachable via TAB #61701 in /test/e2e/specs/site-editor/dataviews-list-layout-keyboard.spec.js
[Flaky Test] Redirection after template creation #60678 in /test/e2e/specs/site-editor/site-editor-url-navigation.spec.js

mirka · 2024-05-28T15:07:27Z

I didn't know about InputBase; I can see it's part of the input-control component. Would it make more sense to make it a separate component if it's going to be used as the basis for others like this (in a follow-up, ofc)? It feels weird it doesn't have its own doc, too. For now, it'd be nice if you could add a bit more information/background on InputBase in the PR description, if possible.

Sorry about that. I added a quick note to the PR description, and a JSDoc on the component for good measure. And good point, we might want to consider extracting out the InputBase from the input-control folder, since it's starting to be used in more places.

DaniGuardiola · 2024-05-28T15:40:59Z

@mirka fwiw, I wanna take a closer look at this InputBase thingy. A while back I looked at it and it seems to be doing a whole lot of not really that much :P - I think there's potential for simplifying or splitting this up into something more manageable and way simpler. Note that I'm including a few different layers that I noticed in input code, so maybe I'm getting things mixed up here and this one's as simple as it gets. In any case, this is for another day, just wanted to make a note!

* Stop exporting unconnected InputBase * CustomSelectControlV2: Use InputBase for styling * Adjust styles * Clean up * Add changelog * Add JSDoc description for InputBase Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: fullofcaffeine <fullofcaffeine@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>

mirka added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Mar 27, 2024

mirka self-assigned this Mar 27, 2024

mirka mentioned this pull request Mar 27, 2024

CustomSelectControl: refactor with ariakit #55023

Open

85 tasks

mirka commented Mar 27, 2024

View reviewed changes

mirka added 4 commits May 24, 2024 21:33

Stop exporting unconnected InputBase

55f5b1f

CustomSelectControlV2: Use InputBase for styling

b24f0f1

Adjust styles

46eeb44

Clean up

a45ee08

mirka force-pushed the custom-select-v2-input-base branch from 17764cd to a45ee08 Compare May 24, 2024 12:42

mirka commented May 24, 2024

View reviewed changes

Add changelog

5c58d6c

mirka marked this pull request as ready for review May 24, 2024 13:28

mirka requested a review from ajitbohra as a code owner May 24, 2024 13:28

mirka requested review from fullofcaffeine and a team May 24, 2024 13:29

fullofcaffeine approved these changes May 27, 2024

View reviewed changes

tyxla approved these changes May 28, 2024

View reviewed changes

Add JSDoc description for InputBase

106f28a

mirka merged commit 6fa3471 into trunk May 28, 2024
63 checks passed

mirka deleted the custom-select-v2-input-base branch May 28, 2024 15:22

github-actions bot added this to the Gutenberg 18.5 milestone May 28, 2024

fullofcaffeine mentioned this pull request May 29, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CustomSelectControlV2: Use `InputBase` for styling #60261

CustomSelectControlV2: Use `InputBase` for styling #60261

mirka commented Mar 27, 2024 •

edited

Loading

mirka Mar 27, 2024

fullofcaffeine May 27, 2024

fullofcaffeine May 27, 2024

mirka Mar 27, 2024

mirka Mar 27, 2024

mirka Mar 27, 2024

fullofcaffeine May 27, 2024 •

edited

Loading

mirka Mar 27, 2024

fullofcaffeine May 27, 2024

mirka May 28, 2024

mirka Mar 27, 2024

mirka May 24, 2024

github-actions bot commented May 24, 2024 •

edited

Loading

fullofcaffeine left a comment •

edited

Loading

tyxla left a comment

tyxla May 28, 2024

mirka May 28, 2024

tyxla May 28, 2024

mirka May 28, 2024

tyxla May 28, 2024

DaniGuardiola May 28, 2024

mirka May 28, 2024

fullofcaffeine May 28, 2024

github-actions bot commented May 28, 2024

mirka commented May 28, 2024

DaniGuardiola commented May 28, 2024 •

edited

Loading

		@@ -14,6 +14,19 @@ export type CustomSelectStore = {

		export type CustomSelectContext = CustomSelectStore \| undefined;

		type CustomSelectSize< Size = 'compact' \| 'default' > = {

CustomSelectControlV2: Use InputBase for styling #60261

CustomSelectControlV2: Use InputBase for styling #60261

Conversation

mirka commented Mar 27, 2024 • edited Loading

What?

Why?

Testing Instructions

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

fullofcaffeine May 27, 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

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

fullofcaffeine left a comment • edited Loading

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

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 May 28, 2024

mirka commented May 28, 2024

DaniGuardiola commented May 28, 2024 • edited Loading

CustomSelectControlV2: Use `InputBase` for styling #60261

CustomSelectControlV2: Use `InputBase` for styling #60261

mirka commented Mar 27, 2024 •

edited

Loading

fullofcaffeine May 27, 2024 •

edited

Loading

github-actions bot commented May 24, 2024 •

edited

Loading

fullofcaffeine left a comment •

edited

Loading

DaniGuardiola commented May 28, 2024 •

edited

Loading