TextControl: support generic props type #64293

meteorlxy · 2024-08-06T03:24:29Z

What?

Fixes #64292

Why?

It would be helpful to infer the onChange type

How?

Making the TextControlProps a generic type
Emit event.target.valueAsNumber or event.target.value according to the type of type prop

Testing Instructions

I've added unit tests for type checking.

Testing Instructions for Keyboard

N/A

Screenshots or screencast

Before

After

github-actions · 2024-08-06T03:24:45Z

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

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

…ol-generic

meteorlxy · 2024-08-10T03:34:56Z

@mirka I just added type checking in the unit tests following your PR #64069. Maybe you could also help to take a look. Thanks!

DaniGuardiola

Thanks for the contribution! Makes sense to me in general, left a few notes.

packages/components/src/text-control/types.ts

DaniGuardiola · 2024-08-10T09:38:14Z

packages/components/src/text-control/types.ts

+	| 'url';
+
+export type TextControlProps<
+	Type extends TextControlPropsType = 'text',


I think we'll want to use the classic naming strategy for type arguments, and go with T and V for consistency with other instances in the package.

DaniGuardiola · 2024-08-10T09:44:06Z

packages/components/src/text-control/types.ts

+
+export type TextControlProps<
+	Type extends TextControlPropsType = 'text',
+	Value extends string | number = Type extends 'number' ? number : string,


It might be better to make this a generic type (e.g. ValueFromType<T> and then use it in place of the NoInfer<Value>'s. The value type is not really a type argument, but a computed type from "Type", which is the actual type argument.

This would also allow passing a Type explicitly without the need to pass a Value (there's no partial inference). Why you'd want to do that? I have no idea. But it's possible, so...

Edit: now that I think about it, since it has a default it might work if omitted, but I think the general point still stands

Plus this allows an invalid combination, e.g. type text and value number. It could be fixed by repeating the condition in the extends clause, but I think it's cleaner to just compute it in each prop with a utility generic type as I suggested.

mirka · 2024-08-12T19:59:00Z

packages/components/src/text-control/types.ts

@@ -14,27 +29,17 @@ export type TextControlProps = Pick<
 	/**
 	 * A function that receives the value of the input.
 	 */
-	onChange: ( value: string ) => void;
+	onChange: ( value: NoInfer< Value > ) => void;


I think this assumption is wrong 🤔 The value that can be retrieved from the change event of an input will always be a string, as far as I can tell. Even if the type is number-like, e.g.number or range. The types for HTMLInputElement confirm this as well.

I was wondering about this, actually. I had assumed that maybe React does something special with the number input to make it work directly with numbers, but it seems like that assumption is wrong. Unfortunately, that means the PR does not make sense and should be closed.

Ummm 🤔 Not sure if you have noticed it... (assume that you ignored it?)

In fact it's updated to make use of the valueAsNumber property for type="number":

It should work well, and should have great browser support: https://caniuse.com/mdn-api_htmlinputelement_valueasnumber

Just added unit tests for this in c2e292a

cc @mirka @DaniGuardiola

Confirming that valueAsNumber browser support is good.

We should still make sure to support a NaN case, as it appears to be a potential return value (see docs):

valueAsNumber
A number that represents the value of the element, interpreted as one of the following, in order: A time value, a number, or NaN if conversion is impossible.

A unit test to cover that scenario would be useful.

Oh, sorry 🤦 But then this is a breaking change, and I don’t think we can ship it on such a widely used and established component, no? I can’t say this is unambiguously a “bug fix” rather than an enhancement.

Yep, this is a big breaking change. I don't think there's nothing to do here unfortunately. At most expose the valueAsNumber attribute and make the generic depend on it. That seems quite convoluted though.

…ol-generic

TextControl: support generic props type

1502277

meteorlxy requested a review from ajitbohra as a code owner August 6, 2024 03:24

docs: add changelog

293e243

mirka added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Aug 9, 2024

mirka requested a review from a team August 9, 2024 12:33

meteorlxy added 3 commits August 10, 2024 10:56

Merge remote-tracking branch 'origin/trunk' into meteorlxy-text-contr…

46333b9

…ol-generic

chore: updates

373b657

chore: updates

da3a79b

chore: update changelog

f593241

DaniGuardiola reviewed Aug 10, 2024

View reviewed changes

mirka reviewed Aug 12, 2024

View reviewed changes

meteorlxy added 2 commits August 13, 2024 17:24

Merge remote-tracking branch 'origin/trunk' into meteorlxy-text-contr…

2a0ec44

…ol-generic

feat: updates

c95dae1

meteorlxy requested review from mirka and DaniGuardiola August 13, 2024 09:46

meteorlxy added 2 commits August 13, 2024 18:14

test: add unit tests

c2e292a

chore: tweaks

678a2b6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TextControl: support generic props type #64293

TextControl: support generic props type #64293

meteorlxy commented Aug 6, 2024 •

edited

Loading

github-actions bot commented Aug 6, 2024 •

edited

Loading

meteorlxy commented Aug 10, 2024

DaniGuardiola left a comment

DaniGuardiola Aug 10, 2024

meteorlxy Aug 13, 2024

DaniGuardiola Aug 10, 2024 •

edited

Loading

meteorlxy Aug 13, 2024

mirka Aug 12, 2024

DaniGuardiola Aug 12, 2024

meteorlxy Aug 13, 2024 •

edited

Loading

meteorlxy Aug 13, 2024 •

edited

Loading

tyxla Aug 13, 2024

mirka Aug 13, 2024

DaniGuardiola Aug 13, 2024

TextControl: support generic props type #64293

Are you sure you want to change the base?

TextControl: support generic props type #64293

Conversation

meteorlxy commented Aug 6, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Aug 6, 2024 • edited Loading

meteorlxy commented Aug 10, 2024

DaniGuardiola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniGuardiola Aug 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

meteorlxy Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

meteorlxy Aug 13, 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

meteorlxy commented Aug 6, 2024 •

edited

Loading

github-actions bot commented Aug 6, 2024 •

edited

Loading

DaniGuardiola Aug 10, 2024 •

edited

Loading

meteorlxy Aug 13, 2024 •

edited

Loading

meteorlxy Aug 13, 2024 •

edited

Loading