fix: upgrade react types to fix component type from memo wrapper

[chrispulsinelli-okta]

OKTA-699592

Summary

Testing & Screenshots

• I have confirmed this change with my designer and the Odyssey Design Team.

[jordankoschei-okta]

Approved, pending a response about those commented-out lines

[bryancunningham-okta]

@chrispulsinelli-okta What problem are we solving here exactly? Conditional rendering? Are we cool with allowing anything to be rendered in the menu item list?

[chrispulsinelli-okta]

A couple of things, actually.
From my usage of this, ReactElement<typeof MenuItem | typeof Divider | typeof ListSubheader> doesn't actually restrict one from using a Button, for example, where we think the generic typing restricts it.

Also, this doesn't allow for null or undefined to be passed in, which are valid JSX values to be passed to render.

ReactNode is a bit more lax in what it allows, allows for nullish values, and also arrays.

[bryancunningham-okta]

Right on. Just wanted to touch base since it was making things more permissive. I don't disagree with the change. Just figured it was worth having a quick touch base about.

[KevinGhadyani-Okta]

Part of the reason we used specific types for children is to ensure other devs know what we expect to be passed in.

Using ReactNode (we shouldn't need NullElement in that case) opens it up so devs can pass anything and won't know exactly what we expect without looking at Storybook.

[chrispulsinelli-okta]

I've mentioned this in another comment in this PR, but will address it here as well.

Part of the reason we used specific types for children is to ensure other devs know what we expect to be passed in.

Understood and I agree with the sentiments that we should be clear about what we expect our users to provide here, but this expression of the type is both hard to read and doesn't actually restrict users to provide the explicit types of react elements listed here.

Personally, I believe it's best to document the expectations in comments and stories and make it much easier to read the type.

Using ReactNode (we shouldn't need NullElement in that case) opens it up so devs can pass anything and won't know exactly what we expect without looking at Storybook.

Using ReactElement also allows users to pass any component of generic type ReactElement. We actually are not preventing them from passing in any other components than the ones explicitly mentioned here.

[KevinGhadyani-Okta]

I'm a bit confused by your statement. I get that the TS doesn't work correctly, but you wrote ReactElement at the end, not ReactNode like the code.

ReactElement is for React components, but I don't think it works for HTML.

ReactNode is any valid React component, HTML element, null, and false if I'm remembering correctly.

[chrispulsinelli-okta]

I'm a bit confused by your statement. I get that the TS doesn't work correctly, but you wrote ReactElement at the end, not ReactNode like the code.

Sorry for the confusion. I'm simply trying to make the point that using ReactElement<typeof FooComponent> doesn't guarantee us that users will provide and only provide a FooComponent.

[KevinGhadyani-Okta]

@chrispulsinelli-okta Is this something we missed in the last upgrade pass on Odyssey?

[KevinGhadyani-Okta]

Are we purposefully removing the alignItems: "flex-end"; from the previous changes for some reason?

[chrispulsinelli-okta]

I didn't remove flex-end alignment.. I actually removed center.
This:

align-items: center;
alignitems: "flex-end";

became this:

alignItems: "flex-end",

(The browser would have cascaded the styles and removed align-items: center anyways)

[KevinGhadyani-Okta]

Why are we making this change to ReactNode from specifying Radio component types as children?

[chrispulsinelli-okta]

Basically, ReactElement<typeof Radio> does nothing to prevent a user from providing something else, like ReactElement<typeof Chekbox>, as a member of the collection provided to the children prop of this RadioGroup.

ReactNode gives us everything Array<ReactElement<typeof Radio>> gives us, as well as the ability to provide undefined and null as values to children, which is the real motivation for my change.

TS doesn't actually enforce the collection of children to be react element's of type Radio; I can provide Checkbox components in lieu of Radio and according to TS, nothing would be wrong with this.

Personally, I'd like for us to recognize that the current provided type isn't actually limiting users to what we want them to provide and is actually making it more difficult to provide actual nullish values that React allows users to provide as children.

Here's how MUI does this, with their expectation of users in a comment, rather than by a type that doesn't work:

[chrispulsinelli-okta]

@KevinGhadyani-Okta Yea, this is an incremental update to things we upgraded previously that would help adoption of Odyssey. I was finding these things as part of my work to upgrade Odyssey in admin ui.