chore(FormSelect): convert examples to TypeScript

[tompsota]

chore(FormSelect): convert examples to TypeScript

Signed-off-by: Tomas Psota tpsota@redhat.com

What: Closes #7475

[patternfly-build]

Preview: https://patternfly-react-pr-7489.surge.sh

A11y report: https://patternfly-react-pr-7489-a11y.surge.sh

[jenny-s51]

Awesome work on this @tompsota ! The examples are looking great. Left some comments below.

In a lot of cases, TS can infer the type of your hooks itself e.g. when you set the value to a string. You don't need the extra <string> type declaration for those!

Just a few easy fixes and otherwise LGTM 👍

[jenny-s51]

Looks 👍👍!

[thatblindgeye]

Great job on the conversions! I also had a couple change requests below, let me know if you have any questions! 🙂

[thatblindgeye]

It doesn't look like this part of the example is working how it may have been intended when originally added.

Rather than trying to set both the validated and value states in the onChange function, it might work better to have the onChange function only call setFormSelectValue like other examples have, and moving the setValidated and checking if formSelectValue is an empty string to a useEffect hook that only runs when the formSelectValue updates. The isEmpty function would just also have to be updated to check that formSelectValue === ''

[tompsota]

Yes, this works better. Thanks for pointing it out.

I also renamed const validated variable so it doesn't collide with state variable.

[thatblindgeye]

This line just needs to be updated so that it sets the validated state correctly; right now it's having the opposite effect of a valid selection rendering the error icon in the example.

Suggested change

	const validationState = !isEmpty() && ValidatedOptions.error;
	const validationState = isEmpty() && ValidatedOptions.error;

[tompsota]

Okay so iiuc, only Select a number option should be invalid? If this is the case, won't it be better to use e.g. 3 as the only valid option and add to corresponding FormSelectOptions label that this is the only valid option? Similarly as in Validated example.

[thatblindgeye]

Okay so iiuc, only Select a number option should be invalid?

That's what it seemed like the intention originally was, at least based on the variable being named isEmpty.

If this is the case, won't it be better to use e.g. 3 as the only valid option and add to corresponding FormSelectOptions label that this is the only valid option? Similarly as in Validated example.

I think either way could work really. While both Validated and Invalid examples are very similar, the main difference just seems to be that the Validate example shows various states of validation instead of only an error like the Invalid example does. As long as that remains true, reusing the onChange from the Validate example and tweaking it to not use helper text or simulate a network call could work (this would also make the useEffect unnecessary)

[tompsota]

Okay, so I kept the original example, it does what we need it to do so it's probably not necessary to make it more complicated.

[thatblindgeye]

Looks good, thank you for working on this! 💪🏼

[tlabaj]

@mcarrano I think we can actually remove the "invalid" example. It is from before we hd the different "validated" states. I don't remember if we kept it for a reason.

[mcarrano]

@mcarrano I think we can actually remove the "invalid" example. It is from before we hd the different "validated" states. I don't remember if we kept it for a reason.

Sounds good to me.

[tlabaj]

LGTM!